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
set browser build in exports #3634
Conversation
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via
or load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## master #3634 +/- ##
=======================================
Coverage 96.54% 96.54%
=======================================
Files 183 183
Lines 6244 6244
Branches 1832 1832
=======================================
Hits 6028 6028
Misses 107 107
Partials 109 109 Continue to review full report at Codecov.
|
We are definitely in agreement here. One thing though is I actually found your suggestion here https://twitter.com/guybedford/status/1270812897959002112 quite interesting. Instead of attaching the ESM build to browser, we make it the default for all non-Node builds. E.g. deno could profit from this as well. It would also be more correct semantically: The browser build of Rollup does not depend on being run in a browser, but the Node build very much depends on Node APIs. What do you think? |
Yes that sounds better to me too, and I think these are the sorts of places where encouraging more scalable patterns can be better. See also my comment here along these lines - https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62#gistcomment-3340001. We have an opportunity right now to define the basic patterns for how users use exports, and ensuring we pick ones that scale will make a huge difference to how platforms can continue to grow in future for JS, so thanks for thinking this one through as well. So if I'm reading the suggestion right - how about we make the |
@sokra you may also be interested in this discussion and PR as well. Note how we're discussing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I'm reading the suggestion right - how about we make the "require" and "import" conditions Node-specific, then have the "default" condition pointing to the browser build instead
Exactly, looks good to me. Will wait a day or two before merging this, though, in case there are some more opinions.
This makes sense for rollup as the browser build is really a pure-js build without using environment-specific api. I would be fine with that. Personally I would make it a bit more specific with One the other hand unsupported runtimes would fail with parsing error anyway. |
I really hope new runtimes just stick with the actual official JavaScript module system. Making this a catch-all is for bundlers which ARE capable of resolving require with ESM. |
It's worth noting the |
Basically |
True. Not all conditions support all module systems through. In node you can't use a ESM in So the question is more: Do we assume that node.js will stay the only exception? I don't think so, as it will probably apply to all implementations which load modules at runtime (where esm loading is by spec async) and support esm and cjs. @guybedford what's about system.js? |
Even if we knew the answer to that question were YES, I am not sure this would be an argument against using |
Yes you are right. Thats why I said it's fine. I only said my personal preference would be different. I personally would only allow cases I know that work and disallow all unknown cases (like an "allowlist"). But allowing all unknown cases and special-casing all known special cases (like a "blocklist") is also a valid way to handle this. It's probably a trade-off between maintance cost vs user errors. Anyway my comment was only about a personal preference. It's wasn't a complain. |
@sokra only allowing cases you know work hits the old UA sniffing pitfalls. The philosophy of universal code that can run anywhere is an important one. Even if it doesn't always work, it allows users to use existing packages in more places where these conventions are adopted. Remember packages on npm are rotting. And the ones being published today with "exports" will rot too. Let them rot gracefully! |
Ok you convinced me. Package rotting is a serious problem and we should tackle this first... I also change the guideline to reflect that. https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62#common-patterns |
This sets the exports
"browser"
condition to point to the correct ES browser build to use for RollupJS.@lukastaegert we had quite a bit of discussion about this stuff previously, but I don't think we had any disagreements about this support mode specifically. If there are any concerns and you want to discuss anything in more detail, I'm happy to leave this till you are back from your holiday and we can arrange a time to chat?
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description