Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RFC: visit exports during dependency tree walk #2

Closed
wants to merge 2 commits into from
Closed

RFC: visit exports during dependency tree walk #2

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8106325
Fix module dependencies export {} from '...' visiting
Oct 8, 2017
d4c77be
Update patch based on Chad's comments (only visit is needed)
Oct 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/com/google/javascript/jscomp/FindModuleDependencies.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.javascript.jscomp.Es6RewriteModules.FindGoogProvideOrGoogModule;
import com.google.javascript.jscomp.deps.ModuleLoader;
import com.google.javascript.jscomp.CompilerInput.ModuleType;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;

Expand Down Expand Up @@ -84,7 +85,18 @@ public boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent)
public void visit(NodeTraversal t, Node n, Node parent) {
if (supportsEs6Modules && n.isExport()) {
moduleType = ModuleType.ES6;

if (n.getBooleanProp(Node.EXPORT_DEFAULT)) {
// export default
} else if (n.getBooleanProp(Node.EXPORT_ALL_FROM)) {
// export * from 'moduleIdentifier';
} else if (n.hasTwoChildren()) {
// export {x, y as z} from 'moduleIdentifier';
Node moduleIdentifier = n.getLastChild();
Node importNode = IR.importNode(IR.empty(), IR.empty(), moduleIdentifier.cloneNode());
importNode.useSourceInfoFrom(n);
parent.addChildBefore(importNode, n);
visit(t, importNode, parent);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely the wrong place. FindModuleDependencies does not actually modify the AST. This needs to be somewhere in Es6RewriteModules.java

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed to when I tried it. Put together an example showing how the bundle changes with and without the patch is here: https://github.com/gregmagolan/closure-export-example.

From what I can tell reading the code, with this change the module in the export { x, y as z } from 'some-module' line gets added as an ordered require for the input:

t.getInput().addOrderedRequire(moduleName);

Without the change, this doesn't happen and the contents of the module don't end up in the bundle.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the ordered required seems correct here. Augmenting the AST though needs to be deferred to other passes.

}
} else if (supportsEs6Modules && n.isImport()) {
moduleType = ModuleType.ES6;
String moduleName;
Expand Down