Skip to content

Commit

Permalink
Only re-write a file as a CommonJS module if it exports a symbol. Req…
Browse files Browse the repository at this point in the history 
…uiring a module should not cause a full re-write.

From @ChadKillingsworth

Closes #1626
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=117279664
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Mar 15, 2016
1 parent dc379d1 commit 56a1395
Show file tree
Hide file tree
Showing 5 changed files with 389 additions and 296 deletions.
39 changes: 26 additions & 13 deletions src/com/google/javascript/jscomp/ProcessCommonJSModules.java
View file
Expand Up @@ -81,11 +81,8 @@ public ProcessCommonJSModules(Compiler compiler, ES6ModuleLoader loader,
public void process(Node externs, Node root) { public void process(Node externs, Node root) {
FindGoogProvideOrGoogModule finder = new FindGoogProvideOrGoogModule(); FindGoogProvideOrGoogModule finder = new FindGoogProvideOrGoogModule();
NodeTraversal.traverseEs6(compiler, root, finder); NodeTraversal.traverseEs6(compiler, root, finder);
if (finder.found) {
return;
}
NodeTraversal NodeTraversal
.traverseEs6(compiler, root, new ProcessCommonJsModulesCallback()); .traverseEs6(compiler, root, new ProcessCommonJsModulesCallback(!finder.found));
} }


String inputToModuleName(CompilerInput input) { String inputToModuleName(CompilerInput input) {
Expand Down Expand Up @@ -187,6 +184,11 @@ private class ProcessCommonJsModulesCallback extends
private List<Node> moduleExportRefs = new ArrayList<>(); private List<Node> moduleExportRefs = new ArrayList<>();
private List<Node> exportRefs = new ArrayList<>(); private List<Node> exportRefs = new ArrayList<>();
Multiset<String> propertyExportRefCount = HashMultiset.create(); Multiset<String> propertyExportRefCount = HashMultiset.create();
private final boolean allowFullRewrite;

public ProcessCommonJsModulesCallback(boolean allowFullRewrite) {
this.allowFullRewrite = allowFullRewrite;
}


@Override @Override
public void visit(NodeTraversal t, Node n, Node parent) { public void visit(NodeTraversal t, Node n, Node parent) {
Expand Down Expand Up @@ -221,7 +223,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
// will be rewritten to: // will be rewritten to:
// //
// if (typeof module == "object" && module.exports) {...} // if (typeof module == "object" && module.exports) {...}
if (n.isIf()) { if (allowFullRewrite && n.isIf()) {
FindModuleExportStatements commonjsFinder = new FindModuleExportStatements(); FindModuleExportStatements commonjsFinder = new FindModuleExportStatements();
Node condition = n.getFirstChild(); Node condition = n.getFirstChild();
NodeTraversal.traverseEs6(compiler, condition, commonjsFinder); NodeTraversal.traverseEs6(compiler, condition, commonjsFinder);
Expand All @@ -243,7 +245,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
visitScript(t, n); visitScript(t, n);
} }


if (n.isGetProp() && if (allowFullRewrite && n.isGetProp() &&
"module.exports".equals(n.getQualifiedName())) { "module.exports".equals(n.getQualifiedName())) {
Var v = t.getScope().getVar(MODULE); Var v = t.getScope().getVar(MODULE);
// only rewrite "module.exports" if "module" is a free variable, // only rewrite "module.exports" if "module" is a free variable,
Expand All @@ -255,7 +257,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
} }
} }


if (n.isName() && EXPORTS.equals(n.getString())) { if (allowFullRewrite && n.isName() && EXPORTS.equals(n.getString())) {
Var v = t.getScope().getVar(n.getString()); Var v = t.getScope().getVar(n.getString());
if (v == null || v.isGlobal()) { if (v == null || v.isGlobal()) {
exportRefs.add(n); exportRefs.add(n);
Expand Down Expand Up @@ -333,9 +335,18 @@ private void visitScript(NodeTraversal t, Node script) {


String moduleName = inputToModuleName(t.getInput()); String moduleName = inputToModuleName(t.getInput());


// Rename vars to not conflict in global scope. boolean hasExports = !(moduleExportRefs.isEmpty() && exportRefs.isEmpty());
NodeTraversal.traverseEs6(compiler, script, new SuffixVarsCallback(
moduleName)); // Rename vars to not conflict in global scope - but only if the script exports something.
// If there are no exports, we still need to rewrite type annotations which
// are module paths
NodeTraversal.traverseEs6(compiler, script,
new SuffixVarsCallback(moduleName, hasExports));

// If the script has no exports, we don't want to output a goog.provide statement
if (!hasExports) {
return;
}


// Replace all refs to module.exports and exports // Replace all refs to module.exports and exports
processExports(script, moduleName); processExports(script, moduleName);
Expand Down Expand Up @@ -581,9 +592,11 @@ private Node getCurrentScriptNode(Node n) {
*/ */
private class SuffixVarsCallback extends AbstractPostOrderCallback { private class SuffixVarsCallback extends AbstractPostOrderCallback {
private final String suffix; private final String suffix;
private final boolean fullRewrite;


SuffixVarsCallback(String suffix) { SuffixVarsCallback(String suffix, boolean fullRewrite) {
this.suffix = suffix; this.suffix = suffix;
this.fullRewrite = fullRewrite;
} }


@Override @Override
Expand All @@ -596,7 +609,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
} }


boolean isShorthandObjLitKey = n.isStringKey() && !n.hasChildren(); boolean isShorthandObjLitKey = n.isStringKey() && !n.hasChildren();
if (n.isName() || isShorthandObjLitKey) { if (fullRewrite && n.isName() || isShorthandObjLitKey) {
String name = n.getString(); String name = n.getString();
if (suffix.equals(name)) { if (suffix.equals(name)) {
// TODO(moz): Investigate whether we need to return early in this unlikely situation. // TODO(moz): Investigate whether we need to return early in this unlikely situation.
Expand Down Expand Up @@ -653,7 +666,7 @@ private void fixTypeNode(NodeTraversal t, Node typeNode) {
String globalModuleName = ES6ModuleLoader.toModuleName(loadAddress); String globalModuleName = ES6ModuleLoader.toModuleName(loadAddress);
typeNode.setString( typeNode.setString(
localTypeName == null ? globalModuleName : globalModuleName + localTypeName); localTypeName == null ? globalModuleName : globalModuleName + localTypeName);
} else { } else if (fullRewrite) {
int endIndex = name.indexOf('.'); int endIndex = name.indexOf('.');
if (endIndex == -1) { if (endIndex == -1) {
endIndex = name.length(); endIndex = name.length();
Expand Down
197 changes: 109 additions & 88 deletions test/com/google/javascript/jscomp/CommandLineRunnerTest.java
View file
Expand Up @@ -60,6 +60,7 @@
* @author nicksantos@google.com (Nick Santos) * @author nicksantos@google.com (Nick Santos)
*/ */
public final class CommandLineRunnerTest extends TestCase { public final class CommandLineRunnerTest extends TestCase {
private static final Joiner LINE_JOINER = Joiner.on('\n');


private Compiler lastCompiler = null; private Compiler lastCompiler = null;
private CommandLineRunner lastCommandLineRunner = null; private CommandLineRunner lastCommandLineRunner = null;
Expand Down Expand Up @@ -1547,43 +1548,48 @@ public void testProcessCJSWithClosureRequires() {
setFilename(3, "app.js"); setFilename(3, "app.js");
test( test(
new String[] { new String[] {
"/** @provideGoog */\n" LINE_JOINER.join(
+ "/** @const */ var goog = goog || {};\n" "/** @provideGoog */",
+ "var COMPILED = false;\n" "/** @const */ var goog = goog || {};",
+ "goog.provide = function (arg) {};\n" "var COMPILED = false;",
+ "goog.require = function (arg) {};", "goog.provide = function (arg) {};",
"goog.require = function (arg) {};"),
"goog.provide('goog.array');", "goog.provide('goog.array');",
"goog.require('goog.array');" LINE_JOINER.join(
+ "function Baz() {}" "goog.require('goog.array');",
+ "Baz.prototype = {" "function Baz() {}",
+ " baz: function() {" "Baz.prototype = {",
+ " return goog.array.last(['asdf','asd','baz']);" " baz: function() {",
+ " }," " return goog.array.last(['asdf','asd','baz']);",
+ " bar: function () {" " },",
+ " return 4 + 4;" " bar: function () {",
+ " }" " return 4 + 4;",
+ "};" " }",
+ "module.exports = Baz;", "};",
"var Baz = require('./Baz');" "module.exports = Baz;"),
+ "var baz = new Baz();" LINE_JOINER.join(
+ "console.log(baz.baz());" "var Baz = require('./Baz');",
+ "console.log(baz.bar());" "var baz = new Baz();",
"console.log(baz.baz());",
"console.log(baz.bar());")
}, },
new String[] { new String[] {
"var goog=goog||{},COMPILED=!1;" LINE_JOINER.join(
+ "goog.provide=function(a){};goog.require=function(a){};", "var goog=goog||{},COMPILED=!1;",
"goog.provide=function(a){};goog.require=function(a){};"),
"goog.array={};", "goog.array={};",
"function Baz$$module$Baz(){}" LINE_JOINER.join(
+ "Baz$$module$Baz.prototype={" "function Baz$$module$Baz(){}",
+ " baz:function(){return goog.array.last(['asdf','asd','baz'])}," "Baz$$module$Baz.prototype={",
+ " bar:function(){return 8}" " baz:function(){return goog.array.last(['asdf','asd','baz'])},",
+ "};" " bar:function(){return 8}",
+ "var module$Baz=Baz$$module$Baz;", "};",
"var module$app={}," "var module$Baz=Baz$$module$Baz;"),
+ " Baz$$module$app=Baz$$module$Baz," LINE_JOINER.join(
+ " baz$$module$app=new Baz$$module$app;" "var Baz = Baz$$module$Baz,",
+ "console.log(baz$$module$app.baz());" " baz = new Baz();",
+ "console.log(baz$$module$app.bar())" "console.log(baz.baz());",
"console.log(baz.bar());")
}); });
} }


Expand All @@ -1603,43 +1609,48 @@ public void testProcessCJSWithClosureRequires2() {


test( test(
new String[] { new String[] {
"/** @provideGoog */" LINE_JOINER.join(
+ "/** @const */ var goog = goog || {};" "/** @provideGoog */",
+ "var COMPILED = false;" "/** @const */ var goog = goog || {};",
+ "goog.provide = function (arg) {};" "var COMPILED = false;",
+ "goog.require = function (arg) {};", "goog.provide = function (arg) {};",
"goog.require = function (arg) {};"),
"goog.provide('goog.array');", "goog.provide('goog.array');",
"goog.require('goog.array');" LINE_JOINER.join(
+ "function Baz() {}" "goog.require('goog.array');",
+ "Baz.prototype = {" "function Baz() {}",
+ " baz: function() {" "Baz.prototype = {",
+ " return goog.array.last(['asdf','asd','baz']);" " baz: function() {",
+ " }," " return goog.array.last(['asdf','asd','baz']);",
+ " bar: function () {" " },",
+ " return 4 + 4;" " bar: function () {",
+ " }" " return 4 + 4;",
+ "};" " }",
+ "module.exports = Baz;", "};",
"var Baz = require('./Baz');" "module.exports = Baz;"),
+ "var baz = new Baz();" LINE_JOINER.join(
+ "console.log(baz.baz());" "var Baz = require('./Baz');",
+ "console.log(baz.bar());" "var baz = new Baz();",
"console.log(baz.baz());",
"console.log(baz.bar());")
}, },
new String[] { new String[] {
"var goog=goog||{},COMPILED=!1;" LINE_JOINER.join(
+ "goog.provide=function(a){};goog.require=function(a){};", "var goog=goog||{},COMPILED=!1;",
"goog.provide=function(a){};goog.require=function(a){};"),
"goog.array={};", "goog.array={};",
"function Baz$$module$Baz(){}" LINE_JOINER.join(
+ "Baz$$module$Baz.prototype={" "function Baz$$module$Baz(){}",
+ "baz:function(){return goog.array.last([\"asdf\",\"asd\",\"baz\"])}," "Baz$$module$Baz.prototype={",
+ "bar:function(){return 8}" "baz:function(){return goog.array.last([\"asdf\",\"asd\",\"baz\"])},",
+ "};" "bar:function(){return 8}",
+ "var module$Baz=Baz$$module$Baz;", "};",
"var module$app={}," "var module$Baz=Baz$$module$Baz;"),
+ "Baz$$module$app=Baz$$module$Baz," LINE_JOINER.join(
+ "baz$$module$app=new Baz$$module$app;" "var Baz = Baz$$module$Baz,",
+ "console.log(baz$$module$app.baz());" " baz = new Baz();",
+ "console.log(baz$$module$app.bar());" "console.log(baz.baz());",
"console.log(baz.bar());")
}); });
} }


Expand All @@ -1652,20 +1663,25 @@ public void testProcessCJSWithES6Export() {
setFilename(1, "app.js"); setFilename(1, "app.js");
test( test(
new String[] { new String[] {
"export default class Foo {" + " bar() { console.log('bar'); }" + "}", LINE_JOINER.join(
"var FooBar = require('./foo');" "export default class Foo {",
+ "var baz = new FooBar.default();" " bar() { console.log('bar'); }",
+ "console.log(baz.bar());" "}"),
LINE_JOINER.join(
"var FooBar = require('./foo');",
"var baz = new FooBar.default();",
"console.log(baz.bar());")
}, },
new String[] { new String[] {
"var module$foo={}," LINE_JOINER.join(
+ "Foo$$module$foo=function(){};" "var module$foo={},",
+ "Foo$$module$foo.prototype.bar=function(){console.log(\"bar\")};" "Foo$$module$foo=function(){};",
+ "module$foo.default=Foo$$module$foo;", "Foo$$module$foo.prototype.bar=function(){console.log(\"bar\")};",
"var module$app={}," "module$foo.default=Foo$$module$foo;"),
+ "FooBar$$module$app=module$foo," LINE_JOINER.join(
+ "baz$$module$app=new FooBar$$module$app.default();" "var FooBar = module$foo,",
+ "console.log(baz$$module$app.bar());" " baz = new FooBar.default();",
"console.log(baz.bar());")
}); });
} }


Expand All @@ -1678,18 +1694,23 @@ public void testES6ImportOfCJS() {
setFilename(1, "app.js"); setFilename(1, "app.js");
test( test(
new String[] { new String[] {
"/** @constructor */ function Foo () {}" LINE_JOINER.join(
+ "Foo.prototype.bar = function() { console.log('bar'); };" "/** @constructor */ function Foo () {}",
+ "module.exports = Foo;", "Foo.prototype.bar = function() { console.log('bar'); };",
"import * as FooBar from './foo';" + "var baz = new FooBar();" + "console.log(baz.bar());" "module.exports = Foo;"),
LINE_JOINER.join(
"import * as FooBar from './foo';",
"var baz = new FooBar();",
"console.log(baz.bar());")
}, },
new String[] { new String[] {
"function Foo$$module$foo(){}" LINE_JOINER.join(
+ "Foo$$module$foo.prototype.bar=function(){console.log(\"bar\")};" "function Foo$$module$foo(){}",
+ "var module$foo=Foo$$module$foo;", "Foo$$module$foo.prototype.bar=function(){console.log(\"bar\")};",
"var module$app={}," "var module$foo=Foo$$module$foo;"),
+ "baz$$module$app$$module$app=new Foo$$module$foo();" LINE_JOINER.join(
+ "console.log(baz$$module$app$$module$app.bar());" "var baz$$module$app = new Foo$$module$foo();",
"console.log(baz$$module$app.bar());")
}); });
} }


Expand Down

0 comments on commit 56a1395

Please sign in to comment.