-
Notifications
You must be signed in to change notification settings - Fork 58
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
Transpile the ESM bundle to ES5 but still use ES module system #106
Conversation
Fixes #99 AFAICT, this seems to be the best solution to what __a lot__ of folks in the community have been experiencing with ESM bundles, referenced from a package's `module` entry, that aren't also transpiled down to ES5 (which is technically in conflict with ESM, which is an ES6 feature). The problem arises because this package states both `module` and `main`, but Webpack defaults `mainFields` to `['browser', 'module', 'main']`, in order. So it will always pick `module` over `main` by default, because it doesn't understand that the build's target is NOT a browser that supports ESM... See webpack/webpack#5756
🦋 Changeset detectedLatest commit: cca4d41 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@khamiltonuk @bparish628 Trying to fix tabbable... I think this should get things fixed for the bundlers. |
This would work in the sense that the code used by webpack would use transpiled code, but the main reason people like esm is because it gets treeshaken out. I was looking at how other libraries (chakra-ui, emotion) do it and it seems that the code is transpiled to the current browser supported by the library in all of the supported module formats. It would be nice if this library also did that, where all formats would be transpiled down to the browser that this library says it supports (ie9). I think most projects use esm so things get treeshaken out (with webpack), not necessarily on what browser's can run that code (that is what babel is for). I think browser support and bundler module definitions are two different things. Browser support is babel and import/exports definitions are the bundler's job. This is your library, so don't want to tell you what to do! Just giving my 2 cents 🙂 |
@bparish628 Thanks for your thoughts. I appreciate it! Sure seems like conflicting intents for ESM bundles, but you make a good point about tree shaking with ESM imports and exports, and the |
I thought about this for a bit. Tree shaking is definitely compelling, and we've done work in this module recently to make it tree-shakable (see #39), so I wouldn't want to thwart that effort. I think I went astray when I equated the module system with the browser target, as you (@bparish628) pointed out:
When I think of ESM as just the module system, then it makes sense to transpile the code inside to the same browser target as the CJS and UMD builds. So that's what I will do! Hopefully that will unbreak things happily. 😄 |
Fixes #99
See webpack/webpack#5756
The initial approach was to add a
browser
target to package.json andpoint it at the UMD build so that bundlers would pick that up instead of
the ESM build, since they (except for Rollup) tend to prefer
browser
first, then
module
(ESM), thenmain
.But that would potentially affect tree-shaking, and we don't want to
hamper that optimizing process.
So this PR transpiles the code in the ESM bundle down to ES5, so that
it runs in IE9+ like the CJS and UMD bundles do, while keeping the
import/export ESM statements.
Features and Bug Fixes
Test coverage added/updated.Typings added/updated.README updated (API changes, instructions, etc.).Changes to dependencies explained.yarn changeset
locally to add one, follow prompts).