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

set browser build in exports #3634

Merged
merged 3 commits into from Jun 17, 2020
Merged

set browser build in exports #3634

merged 3 commits into from Jun 17, 2020

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jun 14, 2020

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:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

@rollup-bot
Copy link
Collaborator

rollup-bot commented Jun 14, 2020

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#rollup-browser-export

or load it into the REPL:
https://rollupjs.org/repl/?circleci=11998

@codecov
Copy link

codecov bot commented Jun 14, 2020

Codecov Report

Merging #3634 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 472b4be...abc728a. Read the comment docs.

@lukastaegert
Copy link
Member

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?

@guybedford
Copy link
Contributor Author

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 "require" and "import" conditions Node-specific, then have the "default" condition pointing to the browser build instead? Note that "require" and "import" are supposed to be fully complementary as I'm aiming to land in nodejs/node#33832 soon, so that changing the "default" for RollupJS right now should have no effect at all to users if implementations are correct.

@guybedford
Copy link
Contributor Author

@sokra you may also be interested in this discussion and PR as well. Note how we're discussing the "default" as a best practice currently.

Copy link
Member

@lukastaegert lukastaegert left a 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.

@sokra
Copy link

sokra commented Jun 15, 2020

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 import instead of default as we can't assume in general that require(esm) is possible in everything else than node.js.

One the other hand unsupported runtimes would fail with parsing error anyway.

@lukastaegert
Copy link
Member

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.

@guybedford
Copy link
Contributor Author

It's worth noting the import condition says nothing about the target module format. It is simply the converse of the require condition in Node.js which itself means that require() was used to load something. It could still be interpreted as any module format by the specific module system for both the import and require conditions. Node.js explicitly makes this distinction clear in that the getFormat loader hook is entirely separated from the resolve hook so loaders in Node.js can make these divergences already today.

@guybedford
Copy link
Contributor Author

Basically import implying a module is not at all the definition of import.

@sokra
Copy link

sokra commented Jun 15, 2020

It's worth noting the import condition says nothing about the target module format. It is simply the converse of the require condition in Node.js which itself means that require() was used to load something. It could still be interpreted as any module format by the specific module system for both the import and require conditions.

True. Not all conditions support all module systems through. In node you can't use a ESM in require. Currently node is the only implementation that has this restriction. Other implementations either don't support require at all or allow using an esm in require.

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?

@lukastaegert
Copy link
Member

Even if we knew the answer to that question were YES, I am not sure this would be an argument against using default. If we use default and this works in the new runtime, nothing needs to be done, otherwise we need to update the package. If we do not use default, then we definitely need to update the package, creating unneeded churn especially if the new environment is really only relevant for some edge cases. At the moment, I would clearly favour default.

@sokra
Copy link

sokra commented Jun 15, 2020

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.

@guybedford
Copy link
Contributor Author

@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!

@sokra
Copy link

sokra commented Jun 17, 2020

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

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.

None yet

4 participants