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

CommonJS: Remove redundant namespace declaration. #1071

Merged
merged 1 commit into from Aug 12, 2015

Conversation

Projects
None yet
3 participants
@mneise
Contributor

mneise commented Aug 12, 2015

Namespace should already be declared and initialized by goog.provide. So instead of emitting this:

goog.provide("module$libs$foo");
var module$libs$foo = {};
module$libs$foo.sayHelloInEnglish = function() {
  return "Hello";
};

this should be emitted:

goog.provide("module$libs$foo");
module$libs$foo.sayHelloInEnglish = function() {
  return "Hello";
};

This is also consistent with the way "normal" Google Closure modules are written.

Adjusted the tests to match changes.

See the mailing list discussion: https://groups.google.com/forum/#!topic/closure-compiler-discuss/t8u_YhL2Onk

@googlebot googlebot added the cla: yes label Aug 12, 2015

newVar.setJSDocInfo(NodeUtil.createConstantJsDoc());
Node newExprResult = IR.exprResult(IR.assign(newName, rhsValue)
.copyInformationFromForTree(ref.getParent()));
newExprResult.setJSDocInfo(NodeUtil.createConstantJsDoc());

This comment has been minimized.

@dimvar

dimvar Aug 12, 2015

Contributor

Please remove this line. @const for variables only makes sense with VAR, not at other assignments.

@@ -100,7 +97,7 @@ public void testExports() {
"goog.provide('module$test');"
+ "goog.require('module$other');"
+ "var name$$module$test = module$other;"
+ "/** @const */ var module$test = function () {};");
+ "/** @const */ module$test = function () {};");

This comment has been minimized.

@dimvar

dimvar Aug 12, 2015

Contributor

Per previous comment, there should be no @const generated here.

@dimvar

This comment has been minimized.

Contributor

dimvar commented Aug 12, 2015

Looks fine. Only a minor comment.

@dimvar dimvar self-assigned this Aug 12, 2015

@mneise

This comment has been minimized.

Contributor

mneise commented Aug 12, 2015

Ah, good to know. Made the changes as suggested. Thank you for reviewing 😃

@@ -244,16 +244,14 @@ private void visitScript(NodeTraversal t, Node script) {
private void processExports(Node script, String moduleName) {
if (hasOneTopLevelModuleExportAssign()) {
// One top-level assign: transform to
// /** @const */ var moduleName = rhs
// /** @const */ moduleName = rhs

This comment has been minimized.

@dimvar

dimvar Aug 12, 2015

Contributor

can you remove const from the comment as well? Sorry, missed that the first time.

@dimvar

This comment has been minimized.

Contributor

dimvar commented Aug 12, 2015

LGTM. One more minor thing to fix and then i'll merge it.

CommonJS: Remove redundant namespace declaration.
Namespace should already be declared and initialized by
goog.provide. Fix test to match changes.
@mneise

This comment has been minimized.

Contributor

mneise commented Aug 12, 2015

Good find! Fixed the comment.

dimvar added a commit that referenced this pull request Aug 12, 2015

Merge pull request #1071 from mneise/cjs-no-var
CommonJS: Remove redundant namespace declaration.

@dimvar dimvar merged commit 9e71b48 into google:master Aug 12, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
cla/google All necessary CLAs are signed

@mneise mneise deleted the mneise:cjs-no-var branch Aug 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment