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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Migrate from AMD to ES modules 馃帀 #4541

Merged
merged 20 commits into from Nov 18, 2019

Conversation

@mgol
Copy link
Member

mgol commented Nov 14, 2019

Summary

Migrate source to ES modules. RequireJS is still used to load tests, this may perhaps be changed in a separate PR.

Reviewing by commit might be the easiest.

@timmywil I'd appreciate especially a review of the build process after my changes as custom compilation works slightly differently now (for example, I had to add an explicit import of ./core/parseHTML.js to jquery.js as otherwise excluding AJAX cut out that module as well and a lot depends on it). If you could locally test some scenarios you had in mind when you initially wrote that & make sure they still work, that'd help.

TODO:

  • Add AMD compilation to amd/
  • (optional) Add back AMD mode to QUnit

Checklist

mgol added 13 commits Oct 23, 2019
ES modules bindings are immutable and we relied on the `ajax/var/nonce.js`
variable being mutable. Workaround it by creating a wrapper object.

We might try to compress it somehow in the future.
The export:
```js
export default jQuery.fn.delay;
```
was being cut out by Rollup as it was unused but due to the possibility of
the `jQuery.fn.delay` access having side effects (e.g. via getters) it left
the following line in:
```js
jQuery.fn.delay;
```
This fails our ESLint setup and takes unnecessary space so this commit removes
the export.
Test files themselves are still loaded via RequireJS as that has to work in
IE 11.
鈥ork)

Custom compilations (excluding/including some modules) is not supported yet.
When custom compilation is used, the version string can get large.
This is necessary so that excluding some non-core modules doesn't exclude
"core/parseHTML.js".
@mgol mgol added this to the 4.0.0 milestone Nov 14, 2019
@mgol mgol requested a review from timmywil Nov 14, 2019
test/jquery.js Outdated Show resolved Hide resolved
@mgol mgol force-pushed the mgol:es-modules branch from 0e8b742 to a4250b2 Nov 14, 2019
@mgol

This comment has been minimized.

Copy link
Member Author

mgol commented Nov 17, 2019

This PR now adds 89 bytes to the minified (non-gzipped) build & 5 bytes to the minified gzipped build. The gzipped difference mostly comes from the addition of the ./core/parseHTML.js import to jquery.js, without it it added 36 bytes to minified (non-gzipped) and actually reduced (!) the minified gzipped build by 10 bytes. This is only a result of re-shuffling the final file, apparently Uglify was compiling the previous one better.

I'm OK with such a delta given the gains from using the modern module format & removing our brittle compile process hacks as it's now mostly relying on Rollup but maybe it's possible to optimize it a little.

@mgol

This comment has been minimized.

Copy link
Member Author

mgol commented Nov 17, 2019

OK, apparently we can go back to -10 bytes minified gzipped (& minimal unminified diff from the current Git compilation) if we move the ./core/parseHTML.js from directly after the ./core.js import to just before the ./ajax/load.js one.

Copy link
Member

gibson042 left a comment

This is so beautiful! 馃槏

src/ajax.js Outdated Show resolved Hide resolved
@mgol mgol removed the Needs review label Nov 18, 2019
@mgol mgol changed the title Core: Migrate to ES modules Core: Migrate from AMD to ES modules 馃帀 Nov 18, 2019
@mgol mgol merged commit d0ce00c into jquery:master Nov 18, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@mgol mgol deleted the mgol:es-modules branch Nov 18, 2019
mgol added a commit to mgol/jquery that referenced this pull request Nov 20, 2019
jQuery source is now authored in ECMAScript modules. Native browser support for
them requires full file names including extensions. Rollup works even if import
paths don't specify extensions, though, so one import slipped through without
such an extension, breaking native browser import of src/jquery.js.

A new ESLint rule using eslint-plugin-import prevents us from regressing on that
front.

Ref jquerygh-4541
Ref 0753201
mgol added a commit that referenced this pull request Nov 25, 2019
jQuery source is now authored in ECMAScript modules. Native browser support
for them requires full file names including extensions. Rollup works even
if import paths don't specify extensions, though, so one import slipped
through without such an extension, breaking native browser import of
src/jquery.js.

A new ESLint rule using eslint-plugin-import prevents us from regressing
on that front.

Also, eslint-plugin-import's no-cycle rule is used to avoid import cycles.

Closes gh-4544
Ref gh-4541
Ref 0753201
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Nov 26, 2019
0 of 4 tasks complete
mgol added a commit to mgol/jquery that referenced this pull request Nov 27, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.
mgol added a commit to mgol/jquery that referenced this pull request Nov 27, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.
mgol added a commit to mgol/jquery that referenced this pull request Nov 27, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.
mgol added a commit to mgol/jquery that referenced this pull request Nov 30, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.
mgol added a commit to mgol/jquery that referenced this pull request Nov 30, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.
mgol added a commit to mgol/jquery that referenced this pull request Nov 30, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.

A "Load with AMD" checkbox was also restored to the QUnit setup. Note that,
contrary to jQuery 3.x, AMD files need to be generated via `grunt amd` or
`grunt` as sources are not authored in ECMAScript modules. To achieve a similar
no-compile experience during jQuery 4.x testing, use the new "Load as modules"
checkbox which works in all supported browsers except for IE & Edge (the legacy,
EdgeHTML-based one).
@mgol mgol mentioned this pull request Nov 30, 2019
2 of 2 tasks complete
mgol added a commit to mgol/jquery that referenced this pull request Nov 30, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.

A "Load with AMD" checkbox was also restored to the QUnit setup. Note that,
contrary to jQuery 3.x, AMD files need to be generated via `grunt amd` or
`grunt` as sources are not authored in ECMAScript modules. To achieve a similar
no-compile experience during jQuery 4.x testing, use the new "Load as modules"
checkbox which works in all supported browsers except for IE & Edge (the legacy,
EdgeHTML-based one).
mgol added a commit to mgol/jquery that referenced this pull request Nov 30, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.

A "Load with AMD" checkbox was also restored to the QUnit setup. Note that,
contrary to jQuery 3.x, AMD files need to be generated via `grunt amd` or
`grunt` as sources are not authored in ECMAScript modules. To achieve a similar
no-compile experience during jQuery 4.x testing, use the new "Load as modules"
checkbox which works in all supported browsers except for IE & Edge (the legacy,
EdgeHTML-based one).
mgol added a commit to mgol/jquery that referenced this pull request Nov 30, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.

A "Load with AMD" checkbox was also restored to the QUnit setup. Note that,
contrary to jQuery 3.x, AMD files need to be generated via `grunt amd` or
`grunt` as sources are not authored in ECMAScript modules. To achieve a similar
no-compile experience during jQuery 4.x testing, use the new "Load as modules"
checkbox which works in all supported browsers except for IE & Edge (the legacy,
EdgeHTML-based one).
mgol added a commit to mgol/jquery that referenced this pull request Dec 2, 2019
jQuery source has been migrated in jquerygh-4541 from AMD to ES modules. To maintain
support for consumers of our AMD modules, this commits adds a task transpiling
the ES modules sources in `src/` to AMD in `amd/`.

A "Load with AMD" checkbox was also restored to the QUnit setup. Note that,
contrary to jQuery 3.x, AMD files need to be generated via `grunt amd` or
`grunt` as sources are not authored in ECMAScript modules. To achieve a similar
no-compile experience during jQuery 4.x testing, use the new "Load as modules"
checkbox which works in all supported browsers except for IE & Edge (the legacy,
EdgeHTML-based one).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.