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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a deterministic depth-first order when managing dependencies #2641

Closed

Conversation

ChadKillingsworth
Copy link
Collaborator

Warning: this is potentially a high-impact change.

Due to the history of ordering dependencies as scripts with goog.provide and goog.require calls, the dependency sorting could produce incorrect results when using ES6 or CommonJS modules. In addition, the regex based parsing of dependencies did not recognize destructuring imports that spanned more than one line - and these are quite common external to Google.

This change begins with entry points and fully parses each dependency as discovered. The order a dependency reference is encountered is preserved and the module sorting later performed now honors that ordering.

Benefits of this change:

  • Avoids a full parse on every input when CommonJS module processing is enabled.
  • ES6 and CommonJS modules no longer generate synthetic goog.require and goog.provide calls.
  • ES6 module transpilation no longer depends on Closure-Library primitives.
  • CommonJS module rewriting no longer depends on ES6 input

Side effects of this change:

  • Multi-threaded parsing will now only apply to externs.

Fixes #2590
Fixes #2535
Fixes #2559
Fixes #2362
Fixes #2247

@thelgevold
Copy link

Thanks @ChadKillingsworth
I just pulled down this PR and confirmed that it fixes the original issue described in #2637

@ChadKillingsworth
Copy link
Collaborator Author

@blickly I haven't been able to create a repo case where base.js is ordered incorrectly. I did add a couple of test cases to prove dependency ordering is not dependent on the user provided input order.

The one oddity I found was https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/JSModuleGraph.java#L542-L545
While I can't figure out why it would cause a problem, the order of entry points matters. And always adding base.js as an entry point seems "odd".

@ChadKillingsworth
Copy link
Collaborator Author

One other note: historical precedence adds moochers to the front of the entry point list and user defined entry points at the end. If moochers are traditionally tests, should this order be reversed?

@blickly
Copy link
Contributor

blickly commented Sep 7, 2017

I'm seeing cases where this is incorrectly ordering base.js after other files. For example, with the following setup.

// bootstrap.js
goog.require('foo');
// foo.js
goog.provide('foo');
goog.setTestOnly();

When running the compiler with sorting but no pruning with the files passed in the order:
--js=bootstrap.js --js=closure/base.js --js=foo.js
we were previously getting the (correct) ordering of
base, foo, bootstrap,
but after this PR, we're getting an ordering of
foo, bootstrap, base

…in a deterministic depth-first order from entry points.

Avoids a full parse on every input when CommonJS module processing is enabled.
ES6 and CommonJS modules no longer generate synthetic goog.require and goog.provide calls.
ES6 Module Transpilation no longer depends on Closure-Library primitives.
@blickly
Copy link
Contributor

blickly commented Sep 13, 2017

OK, looks resolved in the current version. Thanks Chad!
Now I'm just seeing issues that need to be fixed on our end...

@alexeagle
Copy link
Contributor

@blickly what's your priority for merging this? Angular can't release our version 5.0 right now because ES6 modules are broken. We are hoping to cut an RC next week and might need to escalate.
/cc @IgorMinar

@MatrixFrog
Copy link
Contributor

We're working on an issue with some internal tests that have done some unusual things.

@alexeagle
Copy link
Contributor

Any estimate of how many tests, and how broken they are? Do you need help to land the CL?

@blickly
Copy link
Contributor

blickly commented Sep 19, 2017

Sure, never hurts to have more help. Thanks!
I've cc-ed you a task on the internal tracker bug.

@Ramblurr
Copy link

Could this behavior be cause by this bug?

@ChadKillingsworth
Copy link
Collaborator Author

@Ramblurr Yes definitely

@Ramblurr
Copy link

@ChadKillingsworth good to know. I've checked out this PR and while the original error message is gone, I've ran into another one. See the edited portion of the SO question I just added. Could it still be related to this PR?

lauraharker pushed a commit that referenced this pull request Oct 16, 2017
*** Reason for rollback ***

Need to roll this back to unblock rollback of #2641

*** Original change description ***

Always transpile modules, even if the output language requested was ES6 or higher.

Since the compiler outputs a single file, it doesn't make sense to leave the import/export statements alone, unless the input to the compiler was just a single file, which is very rare.

This allows for projects that are using ES6 modules to pass --language_out=ES_2015 or higher.

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=172167855
lauraharker pushed a commit that referenced this pull request Oct 16, 2017
*** Original change description ***

When --dependency_mode is set to STRICT or LOOSE, order dependencies in a deterministic depth-first order from entry points.
Avoids a full parse on every input when CommonJS module processing is enabled.
ES6 and CommonJS modules no longer generate synthetic goog.require and goog.provide calls.
ES6 Module Transpilation no longer depends on Closure-Library primitives.

***

Reopens #2641

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=172176468
@lauraharker
Copy link
Contributor

This PR was rolled back in 22240a5 after it broke some internal projects. @MatrixFrog is working on a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants