Skip to content

Commit

Permalink
Disallow the var name = goog.require(...).name; pattern.
Browse files Browse the repository at this point in the history
The same behavior can be accomplished in ES6 syntax as
  const {name} = goog.require(...);
and is closer to the import syntax of ES6 modules.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124137480
  • Loading branch information
blickly committed Jun 6, 2016
1 parent 96dbbf6 commit 7e5ecf4
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 92 deletions.
7 changes: 0 additions & 7 deletions src/com/google/javascript/jscomp/ClosureCheckModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,6 @@ private void checkRequireCall(NodeTraversal t, Node callNode, Node parent) {
switch (parent.getType()) {
case Token.EXPR_RESULT:
return;
case Token.GETPROP:
// TODO(blickly): Disallow this pattern once destructuring assignment works in the release.
if (parent.getParent().isName()) {
checkRequireCall(t, callNode, parent.getParent());
return;
}
break;
case Token.NAME:
case Token.DESTRUCTURING_LHS:
checkShortGoogRequireCall(t, callNode, parent.getParent());
Expand Down
38 changes: 8 additions & 30 deletions src/com/google/javascript/jscomp/ClosureRewriteModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -818,11 +818,6 @@ private void updateGoogRequire(NodeTraversal t, Node call) {
currentScript.googModuleGettedNamespaces.contains(legacyNamespaceNode.getString());
boolean importHasAlias = NodeUtil.isNameDeclaration(statementNode);
boolean isDestructuring = statementNode.getFirstChild().isDestructuringLhs();
// Is "(.*)goog.require("bar.Foo").Foo;" style?.
boolean immediatePropertyAccess =
call.getParent().isGetProp()
&& call.getGrandparent().isName()
&& NodeUtil.isNameDeclaration(call.getGrandparent().getParent());

// If the current script is a module or the require statement has a return value that is stored
// in an alias then the require is goog.module() style.
Expand Down Expand Up @@ -860,11 +855,8 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
// Delete the goog.require() because we're going to inline its alias later.
statementNode.detachFromParent();
} else if (targetIsNonLegacyGoogModule) {
if (immediatePropertyAccess || !isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
if (!isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
// Rewrite
// "var Foo = goog.require("bar.Foo").Foo;" to
// "var Foo = module$exports$bar$Foo.Foo;"
// or
// "function() {var Foo = goog.require("bar.Foo");}" to
// "function() {var Foo = module$exports$bar$Foo;}"
Node binaryNamespaceName = IR.name(rewriteState.getBinaryNamespace(legacyNamespace));
Expand All @@ -875,27 +867,13 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
statementNode.detachFromParent();
}
} else {
if (immediatePropertyAccess) {
// Rewrite
// "var B = goog.require('B').B;" to
// "goog.require('B'); var B = B.B;"
// because ProcessClosurePrimitives only wants to see simple goog.require() statements.
Node legacyNamespaceName = NodeUtil.newQName(compiler, legacyNamespace);
legacyNamespaceName.srcrefTree(call);
call.getParent().replaceChild(call, legacyNamespaceName);
Node callStatement = IR.exprResult(call);
callStatement.srcref(call);
statementNode.getParent().addChildBefore(callStatement, statementNode);
markConst(statementNode);
} else {
// Rewrite
// "var B = goog.require('B');" to
// "goog.require('B');"
// because even though we're going to inline the B alias,
// ProcessClosurePrimitives is going to want to see this legacy require.
call.detachFromParent();
statementNode.getParent().replaceChild(statementNode, IR.exprResult(call));
}
// Rewrite
// "var B = goog.require('B');" to
// "goog.require('B');"
// because even though we're going to inline the B alias,
// ProcessClosurePrimitives is going to want to see this legacy require.
call.detachFromParent();
statementNode.getParent().replaceChild(statementNode, IR.exprResult(call));
}
if (targetIsNonLegacyGoogModule) {
// Add goog.require() and namespace name to preprocessor table because they're removed
Expand Down
7 changes: 7 additions & 0 deletions test/com/google/javascript/jscomp/ClosureCheckModuleTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,13 @@ public void testLegalAtExport() {
}

public void testIllegalGoogRequires() {
testError(
LINE_JOINER.join(
"goog.module('xyz');",
"",
"var foo = goog.require('other.x').foo;"),
REQUIRE_NOT_AT_TOP_LEVEL);

testError(
LINE_JOINER.join(
"goog.module('xyz');",
Expand Down
55 changes: 0 additions & 55 deletions test/com/google/javascript/jscomp/ClosureRewriteModuleTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -989,61 +989,6 @@ public void testImport() {
"}")});
}

public void testImportProperty1() {
// On a module.
test(
new String[] {
LINE_JOINER.join(
"goog.module('p.A');",
"/** @constructor */ function A() {}",
"exports.A = A;"),
LINE_JOINER.join(
"goog.module('p.C');",
"var A = goog.require('p.A').A;",
"function main() {",
" /** @type {A} */ var a = new A;",
"}")},

new String[] {
LINE_JOINER.join(
"/** @const */ var module$exports$p$A = {};",
"/** @constructor */ function module$contents$p$A_A() {}",
"/** @const */ module$exports$p$A.A = module$contents$p$A_A;"),
LINE_JOINER.join(
"/** @const */ var module$exports$p$C = {};",
"var module$contents$p$C_A = module$exports$p$A.A;",
"function module$contents$p$C_main() {",
" /** @type {module$contents$p$C_A} */ var a = new module$contents$p$C_A;",
"}")});
}

public void testImportProperty2() {
// On a legacy script.
test(
new String[] {
LINE_JOINER.join(
"goog.provide('p.A');",
"/** @constructor */ p.a = function A() {}"),
LINE_JOINER.join(
"goog.module('p.C');",
"var A = goog.require('p.A').A;",
"function main() {",
" /** @type {A} */ var a = new A;",
"}")},

new String[] {
LINE_JOINER.join(
"goog.provide('p.A');",
"/** @constructor */ p.a = function A() {}"),
LINE_JOINER.join(
"/** @const */ var module$exports$p$C = {};",
"goog.require('p.A');",
"/** @const */ var module$contents$p$C_A = p.A.A;",
"function module$contents$p$C_main() {",
" /** @type {module$contents$p$C_A} */ var a = new module$contents$p$C_A;",
"}")});
}

public void testSetTestOnly() {
test(
LINE_JOINER.join(
Expand Down

0 comments on commit 7e5ecf4

Please sign in to comment.