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.

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

Walk dep tree visit exports fix for export {x, y as z} from 'module-identifier' #2726

Closed
wants to merge 1 commit into from

Conversation

gregmagolan
Copy link
Contributor

I needed the following change for a build to work that contained exports such as:

export {x, y as z} from 'some-module';

Files referenced on lines like this were not being included in the bundle. I took a look at the Es6ModuleRewrite code and used that as an example to update FindModuleDependencies.java so that the files referenced in export lines like the one above are visited and end up included in the bundle.

Originally PR'd against the walk-dep tree branch: ChadKillingsworth#2.

@ChadKillingsworth has taken a look at it

Copy link
Collaborator

@ChadKillingsworth ChadKillingsworth left a comment

Choose a reason for hiding this comment

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

Thanks for PRing this. Just a couple notes.

} else if (n.getBooleanProp(Node.EXPORT_ALL_FROM)) {
// export * from 'moduleIdentifier';
// not yet supported by closure ES6 transpilation: 'Wildcard export' is not yet implemented.
} else if (n.hasTwoChildren()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only case we care about since I believe the only way an export has more than one child is if it exports from another module. There's no need to special case export *. While transpilation may not handle this case yet, we still want to add it to the module graph.

Can you simplify this case to that single if test?

// export {x, y as z} from 'moduleIdentifier';
Node moduleIdentifier = n.getLastChild();
Node importNode = IR.importNode(IR.empty(), IR.empty(), moduleIdentifier.cloneNode());
visit(t, importNode, parent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than cloning a node and revisiting, can you refactor out the ES6 import case to a function getModuleNameFromImportPath? Then you can just call that with the 2nd child and add the result to the graph.

@brad4d
Copy link
Contributor

brad4d commented Nov 27, 2017

@ChadKillingsworth thanks for doing the review.
@gregmagolan I believe we're awaiting your responses to Chad's comments.
@blickly would probably be best to import this and do the internal review.

@gregmagolan
Copy link
Contributor Author

@ChadKillingsworth Thanks for the comments. I'll work on your recommended changes today.

@gregmagolan
Copy link
Contributor Author

@ChadKillingsworth Pushed the refactored changes. Ended up adding two functions to capture the common functionality for import and export statements: addEs6ModuleImportToGraph and getEs6ModuleNameFromImportNode. Not sure if the naming is ideal.

Tested it against https://github.com/gregmagolan/closure-export-example.

Also tested with an export * from 'path' statement to ensure the compile still failed with: ERROR - ES6 transpilation of 'Wildcard export' is not yet implemented.

Copy link
Collaborator

@ChadKillingsworth ChadKillingsworth left a comment

Choose a reason for hiding this comment

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

LGTM. Assigning to @blickly for import.

@ChadKillingsworth
Copy link
Collaborator

@gregmagolan can you squash your commits? Importing multiple commits is known to cause problems.

@gregmagolan gregmagolan force-pushed the visit-exports-fix branch 2 times, most recently from 0f6f89d to 540822d Compare November 28, 2017 02:19
@gregmagolan
Copy link
Contributor Author

@gregmagolan can you squash your commits? Importing multiple commits is known to cause problems.

Done

@blickly
Copy link
Contributor

blickly commented Nov 28, 2017

Is it possible to add a test of this?
I think that the exiting CompilerTest.testProperEs6ModuleOrdering is the most similar existing test. See:
https://github.com/google/closure-compiler/blob/master/test/com/google/javascript/jscomp/CompilerTest.java#L1466-L1525

@gregmagolan
Copy link
Contributor Author

Is it possible to add a test of this?

Added testProperEs6ModuleOrderingWithExport

@blickly
Copy link
Contributor

blickly commented Nov 29, 2017

Great, thanks!

Importing now...

@brad4d brad4d closed this in 416a275 Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants