Support full ECMAScript compilation of modules unexpectedly imported from node_modules. #10585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamn
changed the title
benjamn
force-pushed
the
invoke-compileOneJsResource-from-ImportScanner
branch
from
0ecd8d3
to
23cc903
Compare
|
I expect some tests to fail with commit 23cc903, since we're still waiting for babel/babel#9864 to be released in |
Instead of merely supporting ECMAScript module syntax via Reify, we should really be compiling unanticipated modules (typically within node_modules) using the same logic that the rest of the application uses. Note: this processing applies only to .js files for now, since that's what the ImportScanner works with. Should help with #10563.
Now that we have the ability to invoke Babel via compiler plugins for
individual modules encountered by the ImportScanner, it's possible the
ImportScanner will try to compile @babel/runtime/helpers/* modules.
These modules are not safe to recompile because they use native syntax
(such as typeof) in ways that do not need additional transformation or
simulation, and would be broken by applying the usual Babel transforms.
It may bother you that this list of packages is hard-coded, or that it
might grow over time. To ease those concerns, I would say:
1. We can release new versions of the babel-compiler and ecmascript
packages whenever we need to.
2. These particular npm packages belong in this list because Babel
itself assumes they have already been compiled, so there shouldn't be
(m)any other packages that fit that narrow criterion.
In other words, this is just a list of packages that must be left
untouched in order to bootstrap the Babel compiler system, and the
babel-compiler package is where the details of that system primarily
reside, so that's where we should put this list, until/unless we find a
better solution.
Case in point: @babel/runtime/helpers/esm/typeof.js uses ECMAScript module syntax (import, export), but must not be compiled with transforms like @babel/plugin-transform-typeof-symbol, since it's part of the runtime library depended upon by that transform. This logic is an extreme implementation detail for sure, but at least babel-compiler is the only code that needs to know about this complexity.
benjamn
force-pushed
the
invoke-compileOneJsResource-from-ImportScanner
branch
from
23cc903
to
16518da
Compare
|
It turns out those remaining test failures were also legitimate. Because we run Thanks PhantomJS! 👻 |
benjamn
added a commit
that referenced
this pull request
Jun 25, 2019
…ndle. Should fix #10595. Code from the application `node_modules` directory becomes part of the `modules` package, so that it can be imported by any other package that uses the module system, regardless of package load order. Now that we compile code from `node_modules` using `babel-compiler` and `meteor-babel` (#10585), `node_modules` code requires the same runtime environment as any other Meteor JS code. For the most part, this need is satisfied by the `@babel/runtime/helpers/...` modules, which are also defined in the `modules` package because they come from `node_modules`. However, in the legacy bundle, `meteorBabelHelpers.sanitizeForInObject` is used to fix buggy for-in iteration in older Internet Explorers. Thankfully, this extra helper code does not need to be included in the modern or server bundles, but only in legacy code.
benjamn
added a commit
that referenced
this pull request
Jun 25, 2019
…ndle. (#10596) Should fix #10595. Code from the application `node_modules` directory becomes part of the `modules` package, so that it can be imported by any other package that uses the module system, regardless of package load order. Now that we compile code from `node_modules` using `babel-compiler` and `meteor-babel` (#10585), `node_modules` code requires the same runtime environment as any other Meteor JS code. For the most part, this need is satisfied by the `@babel/runtime/helpers/...` modules, which are also defined in the `modules` package because they come from `node_modules`. However, in the legacy bundle, `meteorBabelHelpers.sanitizeForInObject` is used to fix buggy for-in iteration in older Internet Explorers. Thankfully, this extra helper code does not need to be included in the modern or server bundles, but only in legacy code.
benjamn
added a commit
that referenced
this pull request
Jul 1, 2019
In PR #10585, I tried enabling full compiler plugin processing for any modules imported from `node_modules` that were not otherwise handled by compiler plugins, but doing that for all packages was just too slow, not to mention potentially dangerous for modules whose code cannot be safely recompiled by Babel. The primary reasons for wanting to recompile node_modules are 1. to enable "native" ECMAScript `import`/`export` syntax (given that Node.js still does not fully support ESM syntax yet), and 2. to compile standard syntax like `const`, `let`, classes, and arrow functions for legacy browsers. The first goal can be achieved by compiling unanticipated `node_modules` code with Reify, which is much faster than Babel, in part because Reify can avoid doing any parsing when the source contains no `import` or `export` identifiers. This compilation is also cached on disk, so its impact should only be felt during cold builds after a `meteor reset`. The second goal can be accomplished using the new `package.json` configuration option `meteor.nodeModules.recompile`, which causes the recompiled packages to be handled by the normal compiler plugins system, so that the `ImportScanner`'s fallback compilation is not necessary. Before this option was introduced, this recompilation behavior could be achieved by symlinking application directories into `node_modules`, but that always felt like an esoteric hack.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In Meteor 1.8.2, supporting legacy browsers increasingly requires recompiling npm packages that use slightly-too-modern JavaScript syntax like
constandclassand arrow functions.In #10545 and #10550 I added support for ECMAScript module syntax in
node_modules(import,export, and dynamicimport()), but now I realize that's not enough, since many more npm packages use simpler features likeclasssyntax and arrow functions than use module syntax, without even realizing they are excluding older browsers.Instead, this PR uses the standard Meteor compiler plugins system to compile "unanticipated modules," which is the same machinery Meteor applications normally use to compile application code, including the same on-disk and in-memory caching logic. Unanticipated modules are files discovered by the
ImportScannerthat were not originally included byPackageSource#_findSources, and thus not already compiled.While it is possible to get Meteor to compile code imported from
node_modulesusing various tricks such as symlinking, those techniques become more burdensome as more npm packages need to be compiled. If we want to continue claiming that Meteor automatically tailors separate client bundles for modern and legacy browsers (the headline feature of Meteor 1.7), then I think we need to make sure the legacy bundle "just works" by default, and worry about any performance implications of that extra compilation work after gathering real-world feedback.Should solve #10563, and possibly also #9473, since this new logic applies to server code imported from
node_modules, too.