-
-
Notifications
You must be signed in to change notification settings - Fork 916
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 an exports "browser" entry #462
Conversation
Thanks for the contribution @guybedford ! Could you point us to further information regarding which bundlers support this new pattern already (or intend to support it)? |
I posted this issue as I noticed in jspm testing that browser workflows around uuid and "exports" support that they were always bringing in the Node.js builtins. Node.js alludes to it in the docs at https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_conditional_exports. Preact currently supports it in https://unpkg.com/[email protected]/package.json. jspm will be supporting it in the coming release. I'm not sure what Webpack support is looking like currently, @sokra may be able to share some info there. If you'd rather wait for wider adoption feel free to leave this issue open or close, to wait until such a point as you feel it is clear there is a benefit to the project. I personally would recommend adding it though. |
I assume that jspm/jspm-cli#2494 is the corresponding jspm issue. I'm generally happy to be an early adopter of such new semantics, however being an early adopter here also usually means additional effort in dealing with bug reports on this issue tracker. We had this already when adding I would prefer to have some clear intents to implement this from the relevant package managers which so far aren't super clear to me:
So if we manage to reach agreement that this will eventually be widely support, I'm happy to merge this. Since |
@ctavan The
It's actually a little trickier, I can provide more information if you need |
Thanks for providing information on webpack, @evilebottnawi!
Which of the keys will win? For our use case it would be crucial that |
If you do import from ESM module Just not it is WIP and we are open for feedback |
Hopefully the field should be matched in order. So if browser is listed first in the exports object, it will win. If bundlers implement a different precedence order, that should be fixed asap to make sure that the logic applies consistently across environments. :) |
Do we have guaranteed object key order in JSON/JavaScript? |
@ctavan Yes |
From RFC7159 (emphasis mine):
|
https://nodejs.org/api/esm.html
|
@broofa Yeah, there was a discussion about this in the PR for node - it's a compromise between letter-of-the-law and usability. In practice JSON does have ordered keys because that property was viral. Some languages had ordered keys, their devs started to depend on it when reading JSON, and that expectation spread to consumers/producers from other languages. By now all major JSON handling libraries we were able to find were preserving object order or had options to do it, generally speaking. The big gotcha are numerical properties in JS which we explicitly disallow inside of exports because of it. |
Wow. To be honest this doesn’t really make the whole |
@ctavan can you share what about the feature you find surprising here? Tools still have the ability to decide which exports they want to resolve. But by giving precedence ordering allows for cross-tooling determinism in how exports are selected for a given environment vector. The proposal to use ordering was also based on initial feedback from @sokra. |
Deliberately implementing a feature to rely on key-ordering in a context where such ordering is not only unexpected, but expressly not guaranteed is going to cause problems. @devsnek had it right if order is important, use an Array. |
@broofa object order is very much guaranteed in JavaScript, can you clarify what about the ordering seems uncertain? |
Btw, if you prefer to be explicit about the ordering, you can do it: ".": [
{ "browser": "./dist/esm-browser/index.js" },
{ "require": "./dist/index.js" },
{ "import": "./wrapper.mjs" }
], Afaik that should work as well and doesn't depend on object key order at the expense of being a bit more verbose. |
Reordering keys-pairs in JSON shouldn't change the semantics. E.g. If you alphabetize keys (say, for readability, or to limit diff churn) you should be able to do so without getting bit in the ass by it. I can't tell you how thrilled I am to discover that's no longer the case. 😢 |
I see what you mean. We did do quite a bit of research into existing JSON tooling to verify ordering would be compatible with most workflows and were very much aware of the problem but made a decision as a group here. Jan's suggestion is a way to get around that concern if you prefer. It feels like this thread has gone somewhat off track from the initial intention which was to help aid users in adopting that this package is already shipping the "exports" field with browser builds. On the same note, another thing I did notice is that users doing eg How you want to handle these cases is entirely up to you here, the last thing I want to do is force any approach, but just happened to notice these cases so thought I would share the suggestion. |
The json spec doesn't say it's unordered. That's only a interpretation of the website From the spec:
The json spec leave it open for the specification that is defining the use of the JSON to define a semantic to the ordering. This is what the exports spec does. That also means that it's not in general safe to reorder keys in JSON. It depends on the usage of the JSON. |
The spec in question is ECMA-404, quote is from section 6 ("Objects"). See: https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf. Linking here since it took me a second to find the source. |
Another note: this PR should be considered as bugfix (at least for webpack 5 users). |
I agree, we're moving off-topic. Apparently both @broofa and me were just a little shocked to discover that reordering object keys in a JSON file – something we always assumed to be an operation with NO semantic meaning – will suddenly break stuff. Thanks @sokra for bringing to my attention that the spec apparently leaves this open (and thanks to @jkrems for linking the PDF, was just googling for this myself)
Yes, this was a deliberate choice. We already deprecated deep imports 2 major versions ago and removed support for them 1 major version ago. That said, I appreciate that you brought the fact, that the Now back to the actual change (and since @sokra and @evilebottnawi are already in this thread): With the current ordering, webpack will always pick the And then, when bundling for Node.js: Could we end up having the uuid library included twice in the bundle if it is |
That brings another potential issue to my attention: For Node.js builds webpack is supposed to pick up the |
Yes. While you could technically do a separation here with nested conditions this is unneeded and may even hurts bundle size. Since all bundlers support to require esm I would recommend to keep it this way.
Yes this can happen.
Yes. webpack with node target would pickup wrapper.mjs. Not sure if you consider this as problem, but it make sense in my opinion as a node bundle behaves like non-bundled node.
There is a But I see the intend, for systems that support to require esm you want to always use esm. Maybe we need a separate condition here ( |
This is fine and exactly what I want: I always want webpack to pick what's behind the
OK, but only for Node.js right? Not for a browser bundle (if the
While I don't really get the use case of bundling for Node.js (I assume serverless cloud functions?!),
So what we want for this module is the following:
So far we could achieve this by specifying 1. and 2. in Now that Not sure if a new So yes, I do believe that we need an additional key to be able to restore the old webpack 4 behavior. |
For 4, why couldn't it just use |
@ctavan the browser bundle should certainly be as optimized as possible, but the standard Node.js convention is not to worry so much about bundling and code size optimizations as they aren't nearly as critical as they are for the browser environment. If we were talking about 100s of KB here that would be another story, but it seems the numbers are well under 100KB as far as I can tell which is well under a milisecond of startup cost to your Node.js users. As long as Webpack is using the "browser" field for your primary bundling scenarios, and that is well-optimized, then surely that handles the major upgrade path here? |
cc @lukastaegert for rollup I also see the need of adding an additional condition for that case. For webpack it doesn't matter so much, but rollup is really eager to prefer ESM over CJS and not using a ESM wrapper approach. If you want to convince rollup to prefer I would go with a
Technically a For a package.json this could be used: "main": "./dist/index.js",
"module": "./dist/esm-node/index.js",
"browser": "./dist/esm-browser/index.js",
"exports": {
".": {
"browser": "./dist/esm-browser/index.js",
"module": "./dist/esm-node/index.js",
"require": "./dist/index.js",
"default": "./wrapper.mjs"
},
"./package.json": "./package.json"
}, I intentionally omitted the case where a browser build should ship commonjs instead of ESM. Technically this could be covered but is not relevant in practive and only make the package.json more verbose. For the curious... "main": "./dist/index.js",
"module": "./dist/esm-node/index.js",
"browser": {
"./dist/esm-node/index.js": "./dist/esm-browser/index.js",
"./dist/index.js": "./dist/browser.js"
},
"exports": {
".": {
"browser": {
"module": "./dist/esm-browser/index.js",
"require": "./dist/browser.js",
"default": "./dist/esm-browser/index.js",
},
"module": "./dist/esm-node/index.js",
"require": "./dist/index.js",
"default": "./wrapper.mjs"
},
"./package.json": "./package.json"
}, Assuming everybody prefers
Example with the package.json above (independent of exports field support of bundlers):
|
Exactly. There are also other tools that have advanced needs in that direction, e.g. Vite, Snowpack, es-dev-server. I just wrote some ideas up myself that I would expect from such an entry point here: rollup/rollup#3514 (comment) If there is interest, I could try to merge your ideas with mine to form a spec to move things forward. |
Fun fact: At least Vite and es-dev-Server are currently using or at least offering |
Regarding If you want a separate condition for real ESM running in the browser, I would prefer it to use a separate name instead of changing Do we need this anyway? Would you ever pass a different value than for |
As I see it, current browser is like "only do things that would work in a browser but I do not care about module semantics" or in other words it is a messy thing that does not work well for bundler-free dev setup tooling. With your suggestion as "module" is still up for grabs, I would use the opportunity to define some stricter semantics for |
You could change the limitations, but in the end it's up to the user to provide a compatible package. Especially if you change the meaning of There is also the migration aspect. I think there should exist a 1 to 1 mapping from old style to Even when less strict semantic there are a couple benefits of the exports field that are value-able:
I think we are on a good way regarding these limitations anyway: Stage 4 is already recommended for published packages, and this seem to work quite ok.
webpack 5 will error by default when using node built-in modules. We could display more warnings when a module flags itself as "wannabe web esm compatible", but I don't think such a flag fits into the |
True. But personally, I do not see why migration to |
Actually, this is more spot-on: https://twitter.com/mjackson/status/1266527272439181312 |
That being said, I would hope browser dies in favour of something useful that works in browsers. |
Going back to the idea of |
I think moduleOnly may be even more confusing because it’s for environments that treat CommonJS and ESM as somewhat interchangeable. Maybe “bundle” would be more appropriate. But as Guy said further up: this sound like working around tooling restrictions which seems dangerous to encode all over the ecosystem. |
I wouldn't say that My fear is that even if we would define |
|
|
I think you completely missed the point, it is for environments that do not understand CommonJS AT ALL. But so did @sokra:
I think you got this one the wrong way around. A tool can choose to prefer this entry over other alternatives if of course it does not support anything but modules, but also if this would provide a better result. It would also be possible to write a simple CLI validation tool that checks if the |
@sokra @lukastaegert I have just created webpack/webpack#11014 and would like to move the discussion there. I think the discussion we're having here is pretty important to the whole bundler ecosystem and I don't want it to live in the pull request comments of the |
I think it is honestly a complete and utter pipe dream to expect users of npm packages to be able to drop CommonJS support. It will take 10 years for this to happen, and that one dependency that is useful might be CommonJS to be a blocker to such a workflow seems a very fragile and arbitrary development restriction. |
See discussion in #462 Co-authored-by: Guy Bedford <[email protected]>
First of all: the `pkg.exports` spec defines that object key order is being used when resolving for the targeted environment, see uuidjs/uuid#462 (comment) In order for `webpack@5` to pick up the `browser` overrides they must come _before_ `import` and `require`, see https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62 for preliminary documentation. To make full use of `pkg.exports` in node as well we can use package self reference instead of local paths, see the discussion in jkrems/proposal-pkg-exports#47
First of all: the `pkg.exports` spec defines that object key order is being used when resolving for the targeted environment, see uuidjs/uuid#462 (comment) In order for `webpack@5` to pick up the `browser` overrides they must come _before_ `import` and `require`, see https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62 for preliminary documentation. To make full use of `pkg.exports` in node as well we can use package self reference instead of local paths, see the discussion in jkrems/proposal-pkg-exports#47 Unfortunately the browser testing tool polendina doesn't play nice with self reference since it only looks for bare module specifiers in `node_modules` and `node_modules/polendina/node_modules`, see https://github.com/rvagg/polendina/blob/master/lib/webpack.config.js#L28-L39 We would need a way to set up an `resolve.alias` for webpack in polendina to make package self reference work.
It is great to see this package supporting the "exports" field so well.
This adds support for a "browser" entry as well in exports which is a tooling pattern that would be good to encourage as a way to support both the browser field and the exports field together in tools.
Questions / feedback welcome.