- Notifications
You must be signed in to change notification settings - Fork20.6k
Core: Fix the exports setup to make bundlers work with ESM & CommonJS#5429
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
CI is failing because |
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.
I know it's not ready for review yet, but I went ahead and reviewed anyway.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
86e62a3
to1e3ce12
Compare@timmywil the PR is ready for review now. |
Uh oh!
There was an error while loading.Please reload this page.
Very nice! I only have suggestions on the npm scripts |
@timmywil I haven't addressed your feedback yet but I pushed 3 new commits that I had in progress (the first one is titled "Docs: Mention that the CommonJS module doesn't expose named exports"). The first one is a small docs update, the other two are adding more bundler/Node tests for our exports setup - for the slim build & both factory ones. I contemplated simplifying the factory definitions by only exposing the ESM versions but I ultimately decided it's too breaking. People depending on factory behavior can update to our new entry points but they may not be able to switch to ESM. Funny enough, I hit it myself here as our Promises/A+ tests adapter uses the factory build but it's a CommonJS file and needs to expose the API synchronously... The factory setup is still way simpler because there is no default export in ESM and the CommonJS version exports a similar object instead of just the API - and this means adapters are not needed, we can just expose the CommonJS version to Node and the ESM one to bundlers. |
@timmywil I addressed your feedback now, please re-review. |
Uh oh!
There was an error while loading.Please reload this page.
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.
Well done!
mgol commentedMar 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@timmywil FYI, I added an explainer document to the wiki athttps://github.com/jquery/jquery/wiki/jQuery-4-exports-explainer that should help understand the whole |
Also, tweak the Node wrappers & tests a bit without changingthe core of the logic.
The module `node:assert` is deprecated.
We cannot pass a single file via the `module` condition as then`require( "jquery" )` will not return jQuery but instead the module object with`default`, `$` & `jQuery` as keys. Instead:1. For Node.js, detected via the `node` condition: 1. Expose a regular CommonJS version to `require` 2. Expose a tiny wrapper over CommonJS to `import`2. For bundlers, detected via the `module` condition: 1. Expose a regular ESM version to `import` 2. Expose a tiny wrapper over ESM to `require`3. If neither Node.js nor bundlers are detected (no `node` or `module` conditions`): 1. Expose a regular CommonJS version to `require` 2. Expose a regular ESM version to `import`The reasons for such definitions are as follows:1. In Node.js, one can synchronously import from a CommonJS file inside of an ESM one but not vice-versa. To use an ESM file in a CommonJS one, a dynamic import is required and that forces asynchronicity.2. In some bundlers CommonJS is not necessarily enabled - e.g. in Rollup without the CommonJS plugin. Therefore, the ESM version needs to be pure ESM. However, bundlers allow synchronously calling `require` on an ESM file. This is possible since bundlers merge the files before they are passed to the browser to execute and the final bundles no longer contain async import code.3. Bare ESM & CommonJS versions are provided to non-Node non-bundler environments where we cannot assume interoperability between ESM & CommonJS is supported.4. Bare versions cannot be supplied to Node or bundlers as projects using both ESM & CommonJS to fetch jQuery would result in duplicate jQuery instances, leading to increased JS size and disjoint data storage.In addition to the above changes, the `script` condition has been dropped. OnlyWebpack documents this condition and it's not clear when exactly it's triggered.Adding support for a new condition can be added later without a breaking change;removing is not so easy.The `production` & `development` conditions have been removed as well. They werenot really applied correctly; we'd need to provide both of them to each currentleaf which would double the size of the definition for the `.` & `./slim` entrypoints. In jQuery, the only difference between development & production buildsis minification; there are no logic changes so we can pass unminified versionsto all the tooling, expecting minification down the line.Fixesjquerygh-5416
After this commit:1. Node.js always gets the CommonJS version2. Bundlers always get the ESM version3. Other tools take the ESM version when using `import` and the CommonJS when using `require`.The complexity is lower than for the `.` & `./slim` entry points because there'sno default export to handle so node/bundler wrapper files are not necessary.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
The Migrate setup is simpler than Core as it doesn't have the slim or factoryversions, but the core ideas are similar.Refjquery/jquery#5255Refjquery/jquery#5429
The Migrate setup is simpler than Core as it doesn't have the slim or factoryversions, but the core ideas are similar.Refjquery/jquery#5255Refjquery/jquery#5429
The Migrate setup is simpler than Core as it doesn't have the slim or factoryversions, but the core ideas are similar.Fixesjquerygh-524Refjquery/jquery#5255Refjquery/jquery#5429
The Migrate setup is simpler than Core as it doesn't have the slim or factoryversions, but the core ideas are similar.Fixesjquerygh-524Refjquery/jquery#5255Refjquery/jquery#5429
The Migrate setup is simpler than Core as it doesn't have the slim or factoryversions, but the core ideas are similar.Fixesgh-524Closesgh-566Refjquery/jquery#5255Refjquery/jquery#5429
Uh oh!
There was an error while loading.Please reload this page.
Summary
We cannot pass a single file via the
module
condition as thenrequire( "jquery" )
will not return jQuery but instead the module object withdefault
,$
&jQuery
as keys. Instead:node
condition:require
import
module
condition:import
require
node
ormodule
conditions`):
require
import
The reasons for such definitions are as follows:
an ESM one but not vice-versa. To use an ESM file in a CommonJS one,
a dynamic import is required and that forces asynchronicity.
the CommonJS plugin. Therefore, the ESM version needs to be pure ESM.
However, bundlers allow synchronously calling
require
on an ESM file. Thisis possible since bundlers merge the files before they are passed to
the browser to execute and the final bundles no longer contain async import
code.
environments where we cannot assume interoperability between ESM & CommonJS
is supported.
ESM & CommonJS to fetch jQuery would result in duplicate jQuery instances,
leading to increased JS size and disjoint data storage.
In addition to the above changes, the
script
condition has been dropped. OnlyWebpack documents this condition and it's not clear when exactly it's triggered.
Adding support for a new condition can be added later without a breaking change;
removing is not so easy.
The
production
&development
conditions have been removed as well. They werenot really applied correctly; we'd need to provide both of them to each current
leaf which would double the size of the definition for the
.
&./slim
entrypoints. In jQuery, the only difference between development & production builds
is minification; there are no logic changes so we can pass unminified versions
to all the tooling, expecting minification down the line.
For
./factory
&./factory-slim
entry points, only the ESM version is nowsupported. This was done to simplify the setup for this niche use case.
Tests with Rollup (both pure ESM & with the CommonJS plugin) & Webpack have
been added.
I added an explainer document to the wiki athttps://github.com/jquery/jquery/wiki/jQuery-4-exports-explainer that should help understand the whole
exports
definition. It can be helpful especially if we need to make more changes.Fixesgh-5416
Checklist