Complete Rewrite of the ProcessCommonJsModules Pass #2085
Conversation
|
This avoids the pattern highlighted in #2083 as much as possible. There are still cases where that pattern can be generated (as highlighted in the issue), but they aren't prevalent in any real world code I've encountered. |
ChadKillingsworth
force-pushed
the
commonjs-refactor
branch
from
85c4885
to
21a7226
Compare
| private final boolean allowFullRewrite; | ||
| private final ImmutableCollection<Node> exports; | ||
| private List<Node> imports = new ArrayList<>(); | ||
| private List<Node> rewrittenClassExpressions = new ArrayList<>(); |
There was a problem hiding this comment.
Looks like these members can all be final
| while (refChild.getNext() != null & refChild.getNext().isExprResult() && | ||
| refChild.getNext().getFirstChild().isCall() && | ||
| ("goog.require".equals(refChild.getNext().getFirstChild().getFirstChild().getQualifiedName()) || | ||
| "goog.provide".equals(refChild.getNext().getFirstChild().getFirstChild().getQualifiedName()))) { |
There was a problem hiding this comment.
I think these could be simplified with .matchesQualifiedName
If you don't mind the slightly strange name, there's also getFirstFirstChild.
| @@ -78,246 +88,185 @@ public ProcessCommonJSModules(Compiler compiler, boolean reportDependencies) { | |||
|
|
|||
| @Override | |||
| public void process(Node externs, Node root) { | |||
There was a problem hiding this comment.
I was a bit confused at first, just because the root node isn't the usual jsRoot (the parent of all input files). Can we add a Preconditions check that root must be a SCRIPT node?
| if (amdFinder.isFound()) { | ||
| visitAMDIfStatement(n); | ||
| if (n.isGetProp()) { | ||
| String qualifiedName = n.getQualifiedName(); |
There was a problem hiding this comment.
I think these might be slightly simpler as n.matchesQualifiedName
Avoids creating aliases for imports and exports thus removing the dependency on the early inlining pass. Only traverses the AST twice.
ChadKillingsworth
force-pushed
the
commonjs-refactor
branch
from
21a7226
to
03a49c3
Compare
|
@blickly done. |
|
I think the travis failures may be wrong - the tests all pass locally for me. |
|
Yeah, I think I've figured out the breakage and have a fix out. (And yes, it's unrelated to this PR) |
Avoids creating aliases for imports and exports thus removing the dependency on the early inlining pass. Only traverses the AST twice. Closes google#2085 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=136504299
The commonjs module rewriting was a very old pass and had many incremental changes. In addition, it suffered from some of the same problems as the old
goog.modulerewriting in that it relied on the early inling pass to handle all of the aliases created.This is an almost complete rewrite. It had the following goals:
I've tested it on my medium sized project that uses commonjs modules extensively. With this change, the number of warnings created with NTI is drastically reduced.
There's some other ancillary benefits: namely the PolymerPass requires behaviors to be global symbols so it now works with CommonJS modules.