Implement the node module resolution algorithm for CommonJS modules #2094

ChadKillingsworth · 2016-10-22T01:25:20Z

Feature referenced by multiple issues and PRs, but especially by #1773.

Adds support for CommonJS module rewriting to handle named (non-path based) module lookups. This implements Node Module Resolution in the compiler natively.

One note: the compiler does not discover source files but depends on all necessary files to be passed in as source. I have work started for the Java npm version which takes an entry point then discovers and passes all required files.

As it's required by the algorithm, this also adds support for JSON files to be passed to the compiler as source. These files are rewritten to module exports. This rewriting is dependent on the file name ending with ".json". That's the same restrictions node has.

I only enabled this support for CommonJS modules, but it will definitely be wanted for ES6 modules as well.

ChadKillingsworth · 2016-10-22T13:52:49Z

Example usage: https://github.com/ChadKillingsworth/closure-compiler-npm/blob/1c97ac06c428e13a5384256101a3d66feae0ee3e/test/gulp.js#L296-L315

ChadKillingsworth · 2016-10-22T14:10:55Z

cc @samthor This will require changes in the JS version package as well:

brad4d · 2016-10-26T15:42:38Z

@ChadKillingsworth, can you use goog.module instead of goog.provide when rewriting JSON files as modules?

ChadKillingsworth · 2016-10-26T15:55:19Z

Wouldn't it be rewritten twice that way?

brad4d · 2016-10-28T19:01:38Z

@MatrixFrog tells me it doesn't really matter whether goog.module or goog.provide is used here. Continuing review...

brad4d · 2016-10-28T23:57:33Z

This change is causing failures for some internal tests.
I don't yet know why.

brad4d · 2016-10-29T00:24:44Z

Looks like a red herring. The tests were already broken.

brad4d · 2016-10-31T15:32:45Z

src/com/google/javascript/jscomp/Compiler.java

+      try {
+        // JSON objects need wrapped in parens to parse properly
+        input.getSourceFile().setCode("(" + input.getSourceFile().getCode() + ")");
+      } catch (Exception e) {


What exceptions do we expect here exactly?
Why is it OK to just ignore them?

I added the same Error as handling as other parse points use.

brad4d · 2016-10-31T15:34:57Z

src/com/google/javascript/jscomp/Compiler.java

+
+      Node root = input.getAstRoot(this);
+      if (root == null) {
+        continue;


Are there valid cases where root == null? What are they?

My thought was for invalid JSON. Since we wrap the JSON in parens, I think it will always have at least a root node.

brad4d · 2016-10-31T15:43:47Z

src/com/google/javascript/jscomp/RewriteJsonToModule.java

+
+    n.addChildToFront(
+        IR.exprResult(
+            IR.call(IR.getprop(IR.name("goog"), IR.string("provide")), IR.string(moduleName)))


This is for use with Node.js, right?
Shouldn't we be rewriting this to look like a non-JSON Node.js module then, rather than a goog.provide()?
Perhaps Node.js modules get rewritten to look like goog.provide() anyway?
Even if that's so, isn't it safer to rewrite this as a Node.js module & allow the later pass to rewrite it again as a goog.provide(). Otherwise, we're duplicating the logic here & risking inconsistent behavior.

Discussed this yesterday & agreed to leave it as-is.

brad4d · 2016-10-31T15:49:29Z

src/com/google/javascript/jscomp/deps/ModuleLoader.java

+        }
+
+        // Load as a file
+        String fullModulePath = nodeModulesFolder.getKey() + "node_modules/" + requireName;


Shouldn't MODULE_SLASH be used here?

brad4d · 2016-10-31T15:50:23Z

src/com/google/javascript/jscomp/deps/ModuleLoader.java

+    //   /foo/ -> bar/node_modules/baz/foo_bar_baz.js
+    //   /foo/node_modules/bar/ -> baz/foo_bar_baz.js
+    for (String modulePath : modulePaths) {
+      String[] nodeModulesDirs = modulePath.split("/node_modules/");


Use MODULE_SLASH here & several other places below?

…ules. Adds support to rewrite JSON files passed in as source to module exports.

ChadKillingsworth · 2016-11-01T19:16:31Z

@brad4d Review items should now be addressed.

brad4d · 2016-11-02T16:46:42Z

@ChadKillingsworth What's the resolution for the literal slashes in strings? I don't see a response to those questions.

brad4d · 2016-11-02T17:23:10Z

@ChadKillingsworth nevermind about the '/' question. I see now.
The registry always uses '/' & you only need to use MODULE_SLASH for actual file paths.

MatrixFrog · 2016-11-02T18:52:52Z

If we used the wrong slash somewhere I'm sure one of our valiant Windows users will let us know quite soon, so I'm not too worried about it :) (Does Travis run tests on Windows machines? Or can we make it do so if it doesn't already?)

ChadKillingsworth · 2016-11-02T19:02:19Z

Travis doesn't run tests on windows.:( I think for now we're safe with just the "/" character. If someone complains - I'll be happy to fix it.

brad4d · 2016-11-02T20:26:27Z

FYI, internal review appears to be complete.
I'm just waiting on test runs to complete successfully before submitting.

brad4d · 2016-11-02T20:28:02Z

@MatrixFrog should I be concerned about the Travis failure on this PR?

ChadKillingsworth · 2016-11-02T20:28:31Z

I can fix travis magically.

brad4d · 2016-11-03T16:55:05Z

One of the suggested code improvements from our internal tools actually broke the code. :(
I had to undo the change and restart the global presubmit this morning.

brad4d · 2016-11-04T00:45:48Z

Global presubmit looks good, so I'm submitting the change.

…les. Adds support to rewrite JSON files passed in as source to module exports. Closes google#2094 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=138143265

googlebot added the cla: yes label Oct 22, 2016

ChadKillingsworth force-pushed the module-registry branch from d9dbccc to 7099138 Compare October 22, 2016 09:23

ChadKillingsworth changed the title ~~Implement the node module resolution algorithim for CommonJS modules~~ Implement the node module resolution algorithm for CommonJS modules Oct 22, 2016

ChadKillingsworth force-pushed the module-registry branch from 7099138 to 89e0f23 Compare October 24, 2016 12:46

brad4d self-assigned this Oct 26, 2016

brad4d requested changes Oct 31, 2016

View reviewed changes

brad4d mentioned this pull request Nov 1, 2016

Externs for node.js core modules; documentation unclear? google/closure-compiler-js#33

Closed

Implement the node module resolution algorithim for located named mod…

5097def

…ules. Adds support to rewrite JSON files passed in as source to module exports.

ChadKillingsworth force-pushed the module-registry branch from 89e0f23 to 5097def Compare November 1, 2016 19:12

brad4d approved these changes Nov 2, 2016

View reviewed changes

ChadKillingsworth closed this Nov 2, 2016

ChadKillingsworth reopened this Nov 2, 2016

blickly closed this in 8c8a050 Nov 4, 2016

ChadKillingsworth mentioned this pull request Nov 12, 2016

Treat any type annotation with a / character as a module reference #2145

Closed

ChadKillingsworth deleted the module-registry branch November 13, 2016 13:30

Implement the node module resolution algorithm for CommonJS modules #2094

Implement the node module resolution algorithm for CommonJS modules #2094

Uh oh!

Conversation

ChadKillingsworth commented Oct 22, 2016

Uh oh!

ChadKillingsworth commented Oct 22, 2016

Uh oh!

ChadKillingsworth commented Oct 22, 2016

Uh oh!

brad4d commented Oct 26, 2016

Uh oh!

ChadKillingsworth commented Oct 26, 2016

Uh oh!

brad4d commented Oct 28, 2016

Uh oh!

brad4d commented Oct 28, 2016

Uh oh!

brad4d commented Oct 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChadKillingsworth commented Nov 1, 2016

Uh oh!

brad4d commented Nov 2, 2016

Uh oh!

brad4d commented Nov 2, 2016

Uh oh!

MatrixFrog commented Nov 2, 2016

Uh oh!

ChadKillingsworth commented Nov 2, 2016

Uh oh!

brad4d commented Nov 2, 2016

Uh oh!

brad4d commented Nov 2, 2016

Uh oh!

ChadKillingsworth commented Nov 2, 2016

Uh oh!

brad4d commented Nov 3, 2016

Uh oh!

brad4d commented Nov 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants