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

Remove UMD pattern when processing CommonJS. #1048

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@mneise
Contributor

mneise commented Jul 23, 2015

To remove the UMD pattern from a JavaScript library and pull out the
module.exports or exports, check the condition of an if-statement
for an occurrence of either module.exports/exports for CommonJS or
define.amd for AMD. For an if-statement that is checking for CommonJS,
we pull out the then-branch and remove the rest. For an if-statement
that is checking for AMD, we pull out the else-branch (since it could be
checking for CommonJS) and remove the rest.

For example:

var foobar = {
    foo: "bar"
};

if (typeof define === "function" && define.amd) {
    define([], function() {
        return foobar;
    });
} else if (typeof exports === "object") {
    if (true) {
        module.exports = foobar;
    }
} else {
    this.foobar = foobar;
}

will be translated to:

var module$umd_standard = {},
    foobar$$module$umd_standard = {foo:"bar"},
    module$umd_standard = foobar$$module$umd_standard;

Thread on the mailing list:
https://groups.google.com/forum/#!topic/closure-compiler-discuss/-M1HBUn35fs

@googlebot googlebot added the cla: yes label Jul 23, 2015

@newtack

This comment has been minimized.

newtack commented Aug 7, 2015

Has this been merged? If so can you close it, if not can you please merge it?

@concavelenz

This comment has been minimized.

Contributor

concavelenz commented Aug 8, 2015

Assigned to Martin to be sure there isn't a conflict with the work he was doing with the loader.

* This class is relevant for detecting and removing the UMD pattern.
* Checks, if node includes "module.exports" or "exports".
*/
static class FindModuleExportsOrExports extends AbstractPreOrderCallback {

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

Maybe name it DetectModuleExportStatements?

* This class is relevant for detecting and removing the UMD pattern.
* Checks, if node includes "define.amd".
*/
static class FindDefineAMD extends AbstractPreOrderCallback {

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

See above, Detect...?

@@ -222,6 +289,48 @@ private void visitScript(NodeTraversal t, Node script) {
compiler.reportCodeChange();
}
/**
* Rewrite CommonJS part of UMD pattern. Pull out then branch,

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

Nit: could you wrap the comments at 100 chars?

}
/**
* This class is relevant for detecting and removing the UMD pattern.

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

Phrasing? This class detects the UMD pattern by checking if ....

Same above.

}
/**
* Rewrite AMD part UMD pattern. Pull out else branch since it could

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

3rd person voice, Rewrites AMD....

}
private void pullOutBranch(Node branch) {
Node ifNode = branch.getParent();

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

indentation is wrong

testModules(
"var foobar = {foo: 'bar'};" +
"if (typeof module === 'object' && module.exports) {" +
"module.exports = foobar;" +

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

could you indent within the strings here? It's hard to read what this does. E.g. replace this line with: " module.exports = ..." + (note the WS after the first quote).

@@ -140,6 +188,25 @@ public void visit(NodeTraversal t, Node n, Node parent) {
visitRequireCall(t, n, parent);
}
// Check for UMD pattern, pull out CommonJS branch and remove the rest.

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

Could you add some more documentation on what this exactly does? "pull out" is a bit unclear. Maybe give a succinct example of what pattern it detects and rewrites?

}
}
private void pullOutBranch(Node branch) {

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

name it replaceIfStatementWithBranch?

Node ifNode = branch.getParent();
Node p = ifNode.getParent();
Node newNode = branch;
// remove redundant block node

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

Is this required? If not I wouldn't do it, it'll be done by the optimizer later anyway, won't it?

// pull out else branch
pullOutBranch(n.getChildAtIndex(2));
} else {
p.removeChild(n);

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

Comment on what this does?

// that can be reached.
if (n.isIf()) {
FindModuleExportsOrExports commonjsFinder = new FindModuleExportsOrExports();
NodeTraversal.traverse(compiler, n.getFirstChild(), commonjsFinder);

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

pulling out n.getFirstChild() into a variable condition would make this and the code below more readable.

@mprobst

This comment has been minimized.

Collaborator

mprobst commented Aug 13, 2015

Fine with the loader work but needs rebasing. I also left a bunch of style/naming comments, otherwise this is ok.

@mneise

This comment has been minimized.

Contributor

mneise commented Aug 13, 2015

Thank you for reviewing and for the great feedback! I will work on it 😃

* a "module.exports" or "exports" statement.
*/
static class FindModuleExportStatements extends AbstractPreOrderCallback {

This comment has been minimized.

@mneise

mneise Aug 13, 2015

Contributor

Would FindModuleExportStatements be ok as a name instaed of Detect....? There is already an existing class called FindGoogProvideOrGoogModule (https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/ProcessCommonJSModules.java#L97) and I thought it would be ok if they would have slightly similar names.

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

I see, that makes sense!

Node p = ifStatement.getParent();
Node newNode = branch;
// remove redundant block node
if (branch.isBlock() && branch.getChildCount() == 1) {

This comment has been minimized.

@mneise

mneise Aug 13, 2015

Contributor

The output at the end looks the same, but the tests I've added would fail if we would remove this.

This comment has been minimized.

@mprobst

mprobst Aug 13, 2015

Collaborator

OK. Maybe as a small nit add a comment a la // Not strictly necessary, but makes tests more legible.?

@mprobst

This comment has been minimized.

Collaborator

mprobst commented Aug 13, 2015

One more nit, otherwise LGTM.

Remove UMD pattern when processing CommonJS.
Detects UMD pattern when processing JavaScript modules and rewrites it
to make sure the CommonJS "module.exports" or "exports" statements can
be reached. Checks the condition of an if-statement for an occurrence of
either "module.exports"/"exports" for CommonJS or "define.amd" for
AMD. For an if-statement that is checking for CommonJS, we pull out the
then-branch and remove the rest. For an if-statement that is checking
for AMD, we pull out the else-branch (since it could be checking for
CommonJS) and remove the rest.

For example:

var foobar = {
    foo: "bar"
};

if (typeof define === "function" && define.amd) {
    define([], function() {
        return foobar;
    });
} else if (typeof exports === "object") {
    if (true) {
        module.exports = foobar;
    }
} else {
    this.foobar = foobar;
}

will be translated to:

var module$umd_standard = {},
    foobar$$module$umd_standard = {foo:"bar"},
    module$umd_standard = foobar$$module$umd_standard;
Node p = ifStatement.getParent();
Node newNode = branch;
// Remove redundant block node. Not strictly necessary, but makes tests more legible.
if (branch.isBlock() && branch.getChildCount() == 1) {

This comment has been minimized.

@mneise

mneise Aug 13, 2015

Contributor

Good idea to add a comment for this explaining the situation!

@mprobst

This comment has been minimized.

Collaborator

mprobst commented Aug 13, 2015

Merged as aa0a99c.

Thanks for the contribution, Maria!

@mprobst mprobst closed this Aug 13, 2015

@mneise

This comment has been minimized.

Contributor

mneise commented Aug 13, 2015

Thank you for taking the time to review the changes!

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