Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
mgol merged 14 commits intojquery:mainfrommgol:bundler-require-fixes
Mar 11, 2024

Conversation

mgol
Copy link
Member

@mgolmgol commentedMar 2, 2024
edited
Loading

Summary

We cannot pass a single file via themodule 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 thenode condition:
    1. Expose a regular CommonJS version torequire
    2. Expose a tiny wrapper over CommonJS toimport
  2. For bundlers, detected via themodule condition:
    1. Expose a regular ESM version toimport
    2. Expose a tiny wrapper over ESM torequire
  3. If neither Node.js nor bundlers are detected (nonode ormodule
    conditions`):
    1. Expose a regular CommonJS version torequire
    2. Expose a regular ESM version toimport

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 callingrequire 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, thescript condition has been dropped. Only
Webpack 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.

Theproduction &development conditions have been removed as well. They were
not 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 entry
points. 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 now
supported. 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 wholeexports definition. It can be helpful especially if we need to make more changes.

Fixesgh-5416

Checklist

JaneSmith reacted with thumbs up emoji
@mgolmgol added Core Needs review Discuss in MeetingReserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labelsMar 2, 2024
@mgolmgol added this to the4.0.0 milestoneMar 2, 2024
@mgolmgol self-assigned thisMar 2, 2024
@mgolmgol marked this pull request as draftMarch 2, 2024 22:54
@mgol
Copy link
MemberAuthor

mgol commentedMar 2, 2024

CI is failing becausepretest now de facto depends onbuild:all. This means core tests now require a build. I need to think how to approach this.

@mgolmgolforce-pushed thebundler-require-fixes branch fromd3a28fa to7145c00CompareMarch 2, 2024 23:09
Copy link
Member

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

@mgolmgolforce-pushed thebundler-require-fixes branch 3 times, most recently from86e62a3 to1e3ce12CompareMarch 9, 2024 22:36
@mgolmgol marked this pull request as ready for reviewMarch 9, 2024 22:38
@mgolmgol requested a review fromtimmywilMarch 9, 2024 22:39
@mgol
Copy link
MemberAuthor

mgol commentedMar 9, 2024

@timmywil the PR is ready for review now.

@timmywil
Copy link
Member

Very nice! I only have suggestions on the npm scripts

@mgolmgolforce-pushed thebundler-require-fixes branch from1e3ce12 toedd36c6CompareMarch 10, 2024 23:21
@mgol
Copy link
MemberAuthor

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

@mgol
Copy link
MemberAuthor

@timmywil I addressed your feedback now, please re-review.

@mgolmgolforce-pushed thebundler-require-fixes branch fromc8d1468 tocb875a2CompareMarch 11, 2024 12:22
@mgolmgol requested a review fromtimmywilMarch 11, 2024 12:30
Copy link
Member

@timmywiltimmywil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Well done!

@mgol
Copy link
MemberAuthor

mgol commentedMar 11, 2024
edited
Loading

@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 wholeexports definition. It can be helpful especially if we need to make more changes.

@timmywiltimmywil removed Needs review Discuss in MeetingReserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labelsMar 11, 2024
Also, tweak the Node wrappers & tests a bit without changingthe core of the logic.
mgol added12 commitsMarch 12, 2024 00:29
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.
@mgolmgolforce-pushed thebundler-require-fixes branch fromcb875a2 toe8c6046CompareMarch 11, 2024 23:29
@mgolmgol merged commit60f11b5 intojquery:mainMar 11, 2024
@mgolmgol deleted the bundler-require-fixes branchMarch 11, 2024 23:39
Mti-isf

This comment was marked as spam.

@jqueryjquery deleted a comment fromMti-isfAug 5, 2024
@jqueryjquery deleted a comment fromMti-isfAug 5, 2024
mgol added a commit to mgol/jquery-migrate that referenced this pull requestMar 31, 2025
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
mgol added a commit to mgol/jquery-migrate that referenced this pull requestMar 31, 2025
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
mgol added a commit to mgol/jquery-migrate that referenced this pull requestMar 31, 2025
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
mgol added a commit to mgol/jquery-migrate that referenced this pull requestApr 1, 2025
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
mgol added a commit to jquery/jquery-migrate that referenced this pull requestApr 14, 2025
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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timmywiltimmywiltimmywil approved these changes

@Mti-isfMti-isfMti-isf left review comments

Assignees

@mgolmgol

Labels
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

require( "jquery" ) returns a module object when used with Webpack & jQuery 4.0.0-beta
3 participants
@mgol@timmywil@Mti-isf

[8]ページ先頭

©2009-2025 Movatter.jp