-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Support the node module resolution algorithm for ES6 modules. #2130
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
Support the node module resolution algorithm for ES6 modules. #2130
Conversation
|
Sorry for the delay. |
|
After mulling this over a bit, I think it might be better to restrict the PATH_ONLY mode to loading files only and exclude the directory loading logic. I don't think directory loading logic will every be supported by browsers. |
|
@ChadKillingsworth will that be a significant change to what you have so far? |
|
It should not be a significant update. Github has this new feature though - you can push any changes you make back to git@github.com:ChadKillingsworth/closure-compiler.git -> node-modules-es6 branch. I can add my changes on top of that. |
|
That's good to know. Thanks. On Thu, Nov 10, 2016 at 11:06 AM Chad Killingsworth <
|
|
I pushed up a separate commit with the changes. A lot of the tests had modules without a path identifier (didn't start with a We should probably discuss the impact of module path changes at the meeting on Monday. |
fae44fd to
c80f327
Compare
|
@ChadKillingsworth could you go ahead and squash this into one commit? |
c80f327 to
5c8342c
Compare
|
@brad4d done |
|
Thanks, re-importing and testing now. |
|
The runtime_tests/module_tests are failing with this imported. |
|
No failures from running |
|
How can I run the runtime_tests? I always just use the mvn test command. |
|
@ChadKillingsworth In the meantime the problem is occurring with absolute_path_test.js. |
|
Or not... Still some Google-specific command line options there... |
|
I noticed. Trying to create a custom build with those options. |
|
@brad4d I've gotten some of the tests to compile and run - not even close to all the requirements are available externally. Can you list which test files have failures? I'll restrict my work to looking just at those. |
|
My test run bailed after the file I already mentioned above. |
|
@brad4d That test isn't in the open source repo: If you can send it to me via email, I'll see what's going on. |
|
Well, that's interesting. |
|
Right because the absolute path is different within Google than it is here. Until external people can run the tests, I don't think it's worth it to make an external version of that test. |
|
OK, well it's only excluded because it contains a hard-coded path that won't work in the git repo. import s from 'test/com/google/javascript/jscomp/runtime_tests/module_tests/module_test_resources/exportDefault';
function testDefault() {
assertEquals('this is the default export', s);
} |
|
So I can see right away the problem: the path doesn't start with either a '.' or '/'. This is by design. We can change that behavior for PATH_ONLY, but it seems like this is a case we should fix. However if it's a big fix, we can do it in stages. Is there by chance a |
|
I think internally Blaze automatically passes a default --js_module_root flag, yes. |
|
How do we want to normalize this then? Outside of Google other module loaders let you specify roots, but still require the '/' or '.' character preceding. |
|
@ChadKillingsworth |
|
I think I'm now seeing some other test failures likely due to the repo moving forward. |
|
@ChadKillingsworth I think you made a small change that seems to have fixed the problem test in the latest import I did. Yay! |
|
The only failing tests now are 2 belonging to @jart in closure rules. Errors look like this: |
| loadAddress = resolveCommonJsModuleFromRegistry(requireName); | ||
| switch (resolutionMode) { | ||
| case LEGACY: | ||
| loadAddress = resolveJsModuleFile(moduleAddress); |
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.
@ChadKillingsworth, I'd expect LEGACY to preserve the behavior as it was prior to this change, but this is not.
I think I should change this to contain something like the if/else statement above.
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.
LEGACY mode should never resolve a directory. I thought this code path was identical to the old behavior, but it's possible that it's a change. Is there a specific case that this is breaking?
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.
@ChadKillingsworth
The only breakages I know of are the 2 related to closure rules I mentioned above.
I'm not sure of exactly why they fail,
but the LEGACY code path is definitely not defining the same behavior as before this PR,
so I believe the answer lies somewhere in that difference.
Since you said you updated it, I'll grab a fresh copy of this PR.
I'm going to modify it a bit before submission to ensure that the LEGACY code path preserves current behavior.
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.
@brad4d So I missed the last set of comments about the closure-rules. Github page caching just didn't even show them to me. I think one minor tweak ought to fix the Closure-rules issue. It's nice to see this getting close.
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 can even add a test for that case ...
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.
Shiny!
I'd really like to see the PR show a clear preservation of the previous behavior for the LEGACY code path, though, rather than just a fix for this one case.
Could you do something like this?
- Define private
ModuleLoader.legacyResolveJsModule()(& related methods) that completely preserves current behavior in a way that is clear in the diff. - Call that from the
LEGACYcase in the switch statement.
Thanks.
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.
Yeah that's no problem at all. I'll do it this weekend.
422d825 to
8a1fa35
Compare
Adds a --module_resolution flag to specify whether to use the legacy, node or browser module resolution algortithm to locate modules.
8a1fa35 to
5b24488
Compare
|
@brad4d Changes made. The legacy function always returns a module path - even if it doesn't exist. This is how the current functionality works. Both of the other modes return null in this case. |
| if (isAbsoluteIdentifier(requireName) || isRelativeIdentifier(requireName)) { | ||
| loadAddress = resolveCommonJsModuleFileOrDirectory(requireName); | ||
| } else { | ||
| loadAddress = resolveCommonJsModuleFromRegistry(requireName); |
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.
@ChadKillingsworth
I expected to see this if-else in resolveJsModuleLegacy(), probably calling different subroutines, though (e.g. resolveLegacyJsModuleFileOrDirectory())
I believe you are trying to simplify things by eliminating cases that shouldn't apply in LEGACY mode,
but this still makes the change more risky.
I'd rather such simplification be done in a separate change.
|
Latest version imported & essential internal tweaks applied. |
|
This has been submitted internally & will appear in github on our daily export. |
|
@ChadKillingsworth I see you've updated the JS Modules wiki page. Node and Browser resolution mode both say 'This is the only mode available for releases after Feb 2017`. I assume that this should read 'This mode is only available for releases after Dec 2016' (or possibly 'from Feb 2017', assuming there'll be a release containing this change in Feb)? |
|
@probins You should simply read that as the code has yet to be released. Utilizing the behavior requires building the compiler from HEAD. I'll be updating the page again one a release containing the code has occurred. This PR has such a high level of interest, I wanted the documentation out there for people to use it. |
|
I think @probins main point was that I think you copy pasted wrong. |
|
Ahh good catch - fixed. |
Adds a --module_resolution flag to specify whether to use the legacy, node, or browser module resolution algorithm to locate modules. Closes google#2130 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=145837856
Just fail if the file doesn't exist. This check was preventing use of SourceFile.fromCode(). Related to google#2130 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=141941923
Related to google#2130 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=142472421
Adds a --module_resolution flag to specify whether to use the legacy, node, or browser module resolution algorithm to locate modules. Closes google#2130 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=145837856
|
Note for anyone reading in the future, TensorBoard learned the hard way that ES6 modules are grimly fiendish. tensorflow/tensorboard#747 |
Adds a
--module_resolutionflag to allow users to opt-out of loading from node_modules registries. However the path-based algorithm still follows the node module loading spec with regard to file extension priority and utilizing package.json file main enteries for directories.Adds JSON file support if any module processing is enabled.
Note that the JSParser attempts to resolve ES6 module paths and issue loading errors. This seems strange because similar functionality exists in the ProcessEs6Modules pass (which seems more appropriate). I'm not sure why this logic exists in the parser, but I didn't want to move that until I understood the implications.
Fixes #1897 - any file extension is now allowed for either CommonJS or ES6 modules
UPDATE - Final Options
There are now 3 resolution modes - all of which effect both CommonJS and ES6 modules:
LEGACYkeeps the same behavior the compiler has used historically.NODEuses the node module resolution algorithmnode_modulesfolder. Includes the ability to require directories and JSON files.BROWSERmimics the behavior of MS Edge