RFC: visit exports during dependency tree walk #2
Conversation
|
I don't use that form much. Did another file consume the exports? Are you wanting it included just because it exports something? |
…in a tight loop. NodeTraversal have significant initialization cost. Also, avoid repetitive hash lookups when mapping variables to indexes. For a sample build, this moves CoalesceVariableNames from #2 to being google#13 in the cost rank. I'm sure this pass could be a lot faster but this is a easy win. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=170096543
| Node importNode = IR.importNode(IR.empty(), IR.empty(), moduleIdentifier.cloneNode()); | ||
| importNode.useSourceInfoFrom(n); | ||
| parent.addChildBefore(importNode, n); | ||
| visit(t, importNode, parent); |
There was a problem hiding this comment.
Definitely the wrong place. FindModuleDependencies does not actually modify the AST. This needs to be somewhere in Es6RewriteModules.java
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Adding the ordered required seems correct here. Augmenting the AST though needs to be deferred to other passes.
|
Yes. Another file consumes the export. I put together a trivial example of this issue here: https://github.com/gregmagolan/closure-export-example README explains how to build with a PR2641 dist that ends up with a runtime error and how to build with PR2641 + the patch in the PR which ends up working. |
|
Update the PR. Removed some lines that were copied from Es6RewriteModules.java. Only visit call is now made for the How does this look Chad? No more AST modification I'm guessing with those lines removed. I also tried handling the |
|
@gregmagolan Realized I never got back to you. Go ahead and PR this to the main repo and we'll get this in. For the |
|
PR'd against the main repo: google#2726 |
With the recent go/accurately-maintain-script-node-featureSet change, we can make the ASTValidator to confirm after each pass that: 1. Features encountered during AST traversal of a SCRIPT <= compiler's allowable featureSet [Source] (becomes optional) 2. Features encountered during AST traversal of a SCRIPT <= that particular SCRIPT node’s FEATURE_SET (accurate, without overestimation during transpile) 3. every SCRIPT node’s FEATURE_SET <= compiler's allowable featureSet (possible to do) #1 and #2 happened automatically as part of go/accurately-maintain-script-node-featureSet. This CL adds #3 above. PiperOrigin-RevId: 554614152
I needed the following change for a build to work that contained exports such as:
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.
I didn't handle the
export * from 'some-module'case but that one should probably be handled as well.Does this change make sense to you? Can something like this be included in PR google#2641?