Skip to content

Commit

Permalink
Improve CommonJS UMD pattern detection.
Browse files Browse the repository at this point in the history 
Checks to see if the enclosing "if" test references the "module" or "define" names.
Prevents exports nested in IF clauses from being always detected as a UMD pattern when they may just be performing a conditional export.

Closes #2963

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=199227525
  • Loading branch information
@ChadKillingsworth @tjgq
ChadKillingsworth authored and tjgq committed Jun 5, 2018
1 parent dbc0888 commit 9009ecb
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 24 deletions.
80 changes: 59 additions & 21 deletions src/com/google/javascript/jscomp/ProcessCommonJSModules.java
View file
Expand Up @@ -334,10 +334,16 @@ static class UmdPattern {
static class ExportInfo { static class ExportInfo {
final Node node; final Node node;
final Scope scope; final Scope scope;
final boolean isInSupportedScope;


ExportInfo(Node node, Scope scope) { ExportInfo(Node node, Scope scope) {
this.node = node; this.node = node;
this.scope = scope; this.scope = scope;

Node disqualifyingParent =
NodeUtil.getEnclosingNode(
node, (n) -> n.isIf() || n.isHook() || n.isFunction() || n.isArrowFunction());
this.isInSupportedScope = disqualifyingParent == null;
} }
} }


Expand Down Expand Up @@ -575,10 +581,9 @@ public void visit(NodeTraversal t, Node n, Node parent) {
moduleExports.add(new ExportInfo(n, t.getScope())); moduleExports.add(new ExportInfo(n, t.getScope()));


// If the module.exports statement is nested in the then branch of an if statement, // If the module.exports statement is nested in the then branch of an if statement,
// and the test of the if checks for "module" or "define,
// assume the if statement is an UMD pattern with a common js export in the then branch // assume the if statement is an UMD pattern with a common js export in the then branch
// This seems fragile but has worked well for a long time. Node ifAncestor = getOutermostUmdTest(parent);
// TODO(ChadKillingsworth): Discover if there is a better way to detect these.
Node ifAncestor = getOutermostIfAncestor(parent);
if (ifAncestor != null && (NodeUtil.isLValue(n) || isInIfTest(n))) { if (ifAncestor != null && (NodeUtil.isLValue(n) || isInIfTest(n))) {
UmdPattern existingPattern = findUmdPattern(umdPatterns, ifAncestor); UmdPattern existingPattern = findUmdPattern(umdPatterns, ifAncestor);
if (existingPattern != null) { if (existingPattern != null) {
Expand All @@ -598,11 +603,10 @@ public boolean apply(Node node) {
} }
} else if (n.matchesQualifiedName("define.amd")) { } else if (n.matchesQualifiedName("define.amd")) {
// If a define.amd statement is nested in the then branch of an if statement, // If a define.amd statement is nested in the then branch of an if statement,
// and the test of the if checks for "module" or "define,
// assume the if statement is an UMD pattern with a common js export // assume the if statement is an UMD pattern with a common js export
// in the else branch // in the else branch
// This seems fragile but has worked well for a long time. Node ifAncestor = getOutermostUmdTest(parent);
// TODO(ChadKillingsworth): Discover if there is a better way to detect these.
Node ifAncestor = getOutermostIfAncestor(parent);
if (ifAncestor != null if (ifAncestor != null
&& findUmdPattern(umdPatterns, ifAncestor) == null && findUmdPattern(umdPatterns, ifAncestor) == null
&& (NodeUtil.isLValue(n) || isInIfTest(n))) { && (NodeUtil.isLValue(n) || isInIfTest(n))) {
Expand Down Expand Up @@ -635,10 +639,9 @@ && findUmdPattern(umdPatterns, ifAncestor) == null
exports.add(new ExportInfo(n, t.getScope())); exports.add(new ExportInfo(n, t.getScope()));


// If the exports statement is nested in the then branch of an if statement, // If the exports statement is nested in the then branch of an if statement,
// and the test of the if checks for "module" or "define,
// assume the if statement is an UMD pattern with a common js export in the then branch // assume the if statement is an UMD pattern with a common js export in the then branch
// This seems fragile but has worked well for a long time. Node ifAncestor = getOutermostUmdTest(parent);
// TODO(ChadKillingsworth): Discover if there is a better way to detect these.
Node ifAncestor = getOutermostIfAncestor(parent);
if (ifAncestor != null if (ifAncestor != null
&& findUmdPattern(umdPatterns, ifAncestor) == null && findUmdPattern(umdPatterns, ifAncestor) == null
&& (NodeUtil.isLValue(n) || isInIfTest(n))) { && (NodeUtil.isLValue(n) || isInIfTest(n))) {
Expand Down Expand Up @@ -848,8 +851,11 @@ boolean initializeModule() {
return directAssignments < 2; return directAssignments < 2;
} }


/** Find the outermost if node ancestor for a node without leaving the function scope */ /**
private Node getOutermostIfAncestor(Node n) { * Find the outermost if node ancestor for a node without leaving the function scope. To match,
* the test class of the "if" statement must reference "module" or "define" names.
*/
private Node getOutermostUmdTest(Node n) {
if (n == null || NodeUtil.isTopLevel(n) || n.isFunction()) { if (n == null || NodeUtil.isTopLevel(n) || n.isFunction()) {
return null; return null;
} }
Expand All @@ -861,15 +867,37 @@ private Node getOutermostIfAncestor(Node n) {
// When walking up ternary operations (hook), don't check if parent is the condition, // When walking up ternary operations (hook), don't check if parent is the condition,
// because one ternary operation can be then/else branch of another. // because one ternary operation can be then/else branch of another.
if (parent.isIf() || parent.isHook()) { if (parent.isIf() || parent.isHook()) {
Node outerIf = getOutermostIfAncestor(parent); Node outerIf = getOutermostUmdTest(parent);
if (outerIf != null) { if (outerIf != null) {
return outerIf; return outerIf;
} }


return parent; final List<Node> umdTests = new ArrayList<>();
NodeUtil.visitPreOrder(
parent.getFirstChild(),
new NodeUtil.Visitor() {
@Override
public void visit(Node node) {
if (node.isName()
&& (node.getString().equals(MODULE) || node.getString().equals("define"))) {
umdTests.add(node);
}
}
});

// Webpack replaces tests of `typeof module !== 'undefined'` with `true`
if (umdTests.isEmpty() && parent.getFirstChild().isTrue()) {
umdTests.add(parent.getFirstChild());
}

if (!umdTests.isEmpty()) {
return parent;
}

return null;
} }


return getOutermostIfAncestor(parent); return getOutermostUmdTest(parent);
} }


/** Return whether the node is within the test portion of an if statement */ /** Return whether the node is within the test portion of an if statement */
Expand Down Expand Up @@ -1350,7 +1378,8 @@ private void visitExport(NodeTraversal t, ExportInfo export) {
&& root.getParent().getParent().isExprResult() && root.getParent().getParent().isExprResult()
&& rValueVar != null && rValueVar != null
&& (NodeUtil.getEnclosingScript(rValueVar.nameNode) == null && (NodeUtil.getEnclosingScript(rValueVar.nameNode) == null
|| (rValueVar.nameNode.getParent() != null && !rValueVar.isParam()))) { || (rValueVar.nameNode.getParent() != null && !rValueVar.isParam()))
&& export.isInSupportedScope) {
root.getParent().getParent().detach(); root.getParent().getParent().detach();
t.reportCodeChange(); t.reportCodeChange();
return; return;
Expand Down Expand Up @@ -1388,7 +1417,8 @@ private void visitExport(NodeTraversal t, ExportInfo export) {
} else if (root.getNext() != null } else if (root.getNext() != null
&& root.getNext().isName() && root.getNext().isName()
&& rValueVar != null && rValueVar != null
&& rValueVar.isGlobal()) { && rValueVar.isGlobal()
&& export.isInSupportedScope) {
// This is a where a module export assignment is used in a complex expression. // This is a where a module export assignment is used in a complex expression.
// Before: `SOME_VALUE !== undefined && module.exports = SOME_VALUE` // Before: `SOME_VALUE !== undefined && module.exports = SOME_VALUE`
// After: `SOME_VALUE !== undefined && module$name` // After: `SOME_VALUE !== undefined && module$name`
Expand Down Expand Up @@ -1808,10 +1838,11 @@ private String getExportedName(NodeTraversal t, Node n, Var var) {


Node exportedName = getExportedNameNode(export); Node exportedName = getExportedNameNode(export);
// We don't want to handle the export itself // We don't want to handle the export itself
if (exportRValue == n if (export.isInSupportedScope
|| ((NodeUtil.isClassExpression(exportRValue) && (exportRValue == n
|| NodeUtil.isFunctionExpression(exportRValue)) || ((NodeUtil.isClassExpression(exportRValue)
&& exportedName == n)) { || NodeUtil.isFunctionExpression(exportRValue))
&& exportedName == n))) {
return null; return null;
} }


Expand Down Expand Up @@ -1858,9 +1889,16 @@ private String getExportedName(NodeTraversal t, Node n, Var var) {
key = key.getNext(); key = key.getNext();
} }
if (key != null && keyIsExport) { if (key != null && keyIsExport) {
return baseExportName + "." + key.getString(); if (export.isInSupportedScope) {
return baseExportName + "." + key.getString();
} else {
return n.getQualifiedName();
}
} }
} else { } else {
if (!export.isInSupportedScope) {
return n.getQualifiedName();
}
if (var.getNameNode() == exportedName) { if (var.getNameNode() == exportedName) {
String exportPrefix; String exportPrefix;
if (exportBaseQName.startsWith(MODULE)) { if (exportBaseQName.startsWith(MODULE)) {
Expand Down
21 changes: 18 additions & 3 deletions test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java
View file
Expand Up @@ -694,7 +694,8 @@ public void testUMDRemoveIIFE() {
lines( lines(
"/** @const */ var module$test = {};", "/** @const */ var module$test = {};",
"(function(){", "(function(){",
" /** @const */ module$test.default={foo:'bar'};", " var foobar = {foo: 'bar'};",
" /** @const */ module$test.default=foobar;",
"}).call(window);")); "}).call(window);"));


// Can't remove IIFEs when there are sibling statements // Can't remove IIFEs when there are sibling statements
Expand All @@ -714,7 +715,8 @@ public void testUMDRemoveIIFE() {
lines( lines(
"/** @const */ var module$test = {};", "/** @const */ var module$test = {};",
"(function(){", "(function(){",
" /** @const */ module$test.default={foo:\"bar\"};", " var foobar = {foo: 'bar'};",
" /** @const */ module$test.default = foobar;",
"})();", "})();",
"alert('foo');")); "alert('foo');"));


Expand All @@ -736,7 +738,8 @@ public void testUMDRemoveIIFE() {
"/** @const */ var module$test = {};", "/** @const */ var module$test = {};",
"alert('foo');", "alert('foo');",
"(function(){", "(function(){",
" /** @const */ module$test.default={foo:\"bar\"};", " var foobar={foo:\"bar\"};",
" /** @const */ module$test.default=foobar;",
"})();")); "})();"));


// Annotations for local names should be preserved // Annotations for local names should be preserved
Expand Down Expand Up @@ -1208,4 +1211,16 @@ public void testWebpackAMDModuleShim() {
" function(module) { return module; };"))); " function(module) { return module; };")));
test(inputs, expecteds); test(inputs, expecteds);
} }

public void testUMDRequiresIfTest() {
testModules(
"test.js",
lines("var foobar = {foo: 'bar'}; if (foobar) { module.exports = foobar; }"),
lines(
"/** @const */ var module$test = {};",
"var foobar$$module$test={foo:\"bar\"};",
"if(foobar$$module$test) {",
" /** @const */ module$test.default = foobar$$module$test;",
"}"));
}
} }

0 comments on commit 9009ecb

Please sign in to comment.