Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ESM and rollup build #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hugo-vrijswijk
Copy link

@hugo-vrijswijk hugo-vrijswijk commented Nov 20, 2024

First of all, thanks for this awesome library!

Fixes #94

Adds a rollup config file to build ESM, CJS, and UMD bundles. Based on the source in src/fuzzysort.js.

Checks in the new esm file and the updated cjs and umd files.

Also updates the package.json to include module and browser fields for the ESM and UMD bundles. This lets bundlers, node or CDN's know which version to use.

I've tried to keep things fairly streamlined. Running 'test' automatically builds the files. The src/fuzzysort.js and fuzzysort.mjs files are extremely similar, but that seemed better than the confusion of knowing which file in the root to edit and having rollup potentially overwrite your changes. I've also kept the build files checked in, to keep things simple. A nice future improvement I'm willing to contribute would be github actions CI and creating a PR when the files drift?

@blq
Copy link

blq commented Nov 22, 2024

IMHO you have added a bit too much new pretty-print formatting, making the diff larger than it has to be. Can you reduce that a bit to zone in on just the top-level?

@hugo-vrijswijk
Copy link
Author

hugo-vrijswijk commented Nov 22, 2024

IMHO you have added a bit too much new pretty-print formatting, making the diff larger than it has to be. Can you reduce that a bit to zone in on just the top-level?

Yeah, sure. I admit some of it was my editor auto-formatting by itself. What makes the diff difficult is that the source file is now src/fuzzysort.js instead of fuzzysort.js. So the easiest to diff is probably by comparing src/fuzzysort.js with an older version from clipboard or something.

I've pushed a new version that minimizes formatting changes. The diff is still weird, but that is due to the new file not being compared with the old source. Comparing fuzzysort.js from master and src/fuzzysort.js on this branch is the easiest

export default defineConfig([
{
input: 'src/fuzzysort.js',
output: { banner, exports, file: pkg.browser, format: 'umd', name: 'fuzzysort' },
Copy link

@blq blq Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while you're at it I suggest also enabling source map output to help debugging.
https://rollupjs.org/configuration-options/#output-sourcemap

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For just the minified file? Or all? The sourcemap is quite big to be checked in IMO

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean check in, but generate in build. At least there's no source map comment at the end of the fuzzsort.min.js file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the build output is checked in now. I didn't want to change that compared to how it is now. If I enable sourcemaps a .map file would have to be checked in for each file, since the output is also checked in

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm, maybe a compromise is to add flag to generate .map files but don't check them in? With reasonable assumption users will run a real build themselves.
And change back to the non-minified version in the test.html for the quick tests.

Copy link
Author

@hugo-vrijswijk hugo-vrijswijk Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I've enabled sourcemaps and added the maps to the gitignore. Adding the non-minified version for the tests is a bit tricky since the non-minified versions are ESM and CJS, not IIFE format. Debugging does work great with sourcemaps and the minified files now

@blq
Copy link

blq commented Nov 23, 2024

IMHO you have added a bit too much new pretty-print formatting, making the diff larger than it has to be. Can you reduce that a bit to zone in on just the top-level?

Yeah, sure. I admit some of it was my editor auto-formatting by itself. What makes the diff difficult is that the source file is now src/fuzzysort.js instead of fuzzysort.js. So the easiest to diff is probably by comparing src/fuzzysort.js with an older version from clipboard or something.

I've pushed a new version that minimizes formatting changes. The diff is still weird, but that is due to the new file not being compared with the old source. Comparing fuzzysort.js from master and src/fuzzysort.js on this branch is the easiest

When comparing to the /src version it's much better now yes.
Small thing I noticed is the github link and version nr at the top comment is missing.

@hugo-vrijswijk
Copy link
Author

Those are added to the build output. That way the version number is always up-to-date

@blq
Copy link

blq commented Nov 24, 2024

Those are added to the build output. That way the version number is always up-to-date

ah, ok

Fixes farzher#94

Adds a rollup config file to build ESM, CJS, and UMD bundles. Based on the source in src/fuzzysort.js.

Checks in the new esm file and the updated cjs and umd files.

Also updates the package.json to include `module` and `browser` fields for the ESM and UMD bundles.
@hugo-vrijswijk
Copy link
Author

@farzher could you take a look? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deno/ESM support
2 participants