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
fix(deconflictChunk): Deconflict multiple index
imports for ES format using nested export star statements
#3435
fix(deconflictChunk): Deconflict multiple index
imports for ES format using nested export star statements
#3435
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3435 +/- ##
=======================================
Coverage 95.01% 95.01%
=======================================
Files 171 171
Lines 5834 5838 +4
Branches 1722 1723 +1
=======================================
+ Hits 5543 5547 +4
Misses 157 157
Partials 134 134
Continue to review full report at Codecov.
|
src/utils/deconflictChunk.ts
Outdated
interop: boolean | ||
) { | ||
for (const chunkOrExternalModule of dependencies) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this logic from deconflictImportsOther
and only applied with the index
variable name to account for export * from './inner'
statements. However, I don't know what other implications this has! 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look, thanks already for your work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your proposed fix will only work for your example as the variable is only called "index" because your file is called "index". A general fix would be to run this for ALL dependencies, however this will cause a lot of test regressions (as you may have noticed) due to a lot of unnecessary renames. The core of the problem is that reexporting a namespace is rendered (in es.ts
) as importing the namespace as some variable and then exporting this variable, and this variable is not considered for deconflicting. A "nice" fix would be much more involved, but maybe this extends the scope of the bug fix. A simple hot "blanket" fix with only few regressions would be to unconditionally deconflict the chunk variable names for all dependencies, but only do it if we are preserving modules. Maybe you could do that and add a comment what is fixed here and that this solution is not yet ideal (so that it is not forgotten)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it is only necessary for preserveModules is that these kinds of reexports will not occur for other formats. Basically this line is responsible:
Lines 386 to 388 in 3ef6f07
if (this.graph.preserveModules && variable instanceof NamespaceVariable) { | |
return '*'; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I will do that and can update the specs to see what that impacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I did was only apply this logic for es
and preserveModules: true
which lessens the impact so it doesn't impact current SystemJS behavior. Am I right in assuming this shouldn't really take effect for SystemJS? Otherwise it was affecting a lot more tests (56 vs. <10).
@@ -0,0 +1,4 @@ | |||
import * as index from './module-a/v1/index.js'; | |||
export { index as ModuleA_V1 }; | |||
import * as index$1 from './module-b/v1/index.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main goal of this PR, to deconflict the index
imports.
index
imports for ES format using nested export star statements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment
Thanks @lukastaegert, updated per your guidance. Implications for consumers that I see right now:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good! As the change is only applied to preserveModules, I do not think the cost is very dramatic as preserveModules as usually not used in situations where you directly bundle for web but rather in library scenarios where a second bundler pass will fix the naming anyway.
} | ||
} | ||
deconflictImportsEsmOrSystem(usedNames, imports, dependencies, interop); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and easy to get back to for a deeper fix 👍
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers: Fixes #3365
Description
This is my proposed fix with a test showing the unexpected output of using
preserveModules: true
with ES output and exporting multiple index aliases withinmain.js
which reproduces the issue #3365I modified
deconflictImportsEsm
because what seemed to be happening was theindex
variables weren't being detected asusedNames
on the rootindex.js
file, so they were not being deconflicted. This is OK with System output but not with ES output.I moved the
dependencies
logic fromdeconflictImportsOther
intodeconflictImportsEsm
which fixed the issue. However, I wrapped it in a condition to only apply when the variable name isindex
as without that it seemed to be renaming a lot of variables unecessarily.To be honest, I don't know what other implications this has so I'm leaning on the expertise of the team to provide feedback on this.