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

Bundle module.js in ES5 #132

Merged
merged 4 commits into from
Nov 27, 2020
Merged

Bundle module.js in ES5 #132

merged 4 commits into from
Nov 27, 2020

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Nov 26, 2020

Changes

Turns out dist/module.js was not bundled in es5, which would cause issues in IE11. An user pointed this out in a comment under an old commit.

This is an attempt towards fixing this + adding tests for the future.

Note: I know next to nothing about library bundling in node world, but the solution seems still less than ideal. @mariusandra do you know any libraries we could 'borrow' inspiration from on how to bundle things properly?

Checklist

  • Tests for new code (if applicable)
  • TypeScript definitions (module.d.ts) updated and in sync with library exports (if applicable)

@mariusandra
Copy link
Collaborator

Hey, this is actually how you should do it. Whenever you need to change the targeted ES version somehow, you just add babel and preferably via a bundler plugin. Everything seems fine to me!

Surprisingly, this also reduced the unminified bundle size from 170KB to 164KB. Normally adding babel makes things bigger, so this was interesting to see.

I added two quick changes 1) removed "npx" from the build script in package.json (all node_modules/.bin scripts are automatically in $PATH) and 2) I noticed when going through module.js that the full package.json was inlined, so I changed the import to just take the version.

@macobo macobo merged commit 6e879fe into master Nov 27, 2020
@macobo macobo deleted the module-bundling branch November 27, 2020 09:01
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.

2 participants