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

fix(deconflictChunk): Deconflict multiple index imports for ES format using nested export star statements #3435

Merged

Conversation

kamranayub
Copy link
Contributor

@kamranayub kamranayub commented Mar 10, 2020

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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 within main.js which reproduces the issue #3365

I modified deconflictImportsEsm because what seemed to be happening was the index variables weren't being detected as usedNames on the root index.js file, so they were not being deconflicted. This is OK with System output but not with ES output.

I moved the dependencies logic from deconflictImportsOther into deconflictImportsEsm which fixed the issue. However, I wrapped it in a condition to only apply when the variable name is index 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.

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #3435 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
src/utils/deconflictChunk.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ef6f07...3e97dee. Read the comment docs.

@kamranayub kamranayub marked this pull request as ready for review March 10, 2020 15:53
interop: boolean
) {
for (const chunkOrExternalModule of dependencies) {
Copy link
Contributor Author

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! 🤔

Copy link
Member

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!

Copy link
Member

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)?

Copy link
Member

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:

rollup/src/Chunk.ts

Lines 386 to 388 in 3ef6f07

if (this.graph.preserveModules && variable instanceof NamespaceVariable) {
return '*';
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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';
Copy link
Contributor Author

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.

@kamranayub kamranayub changed the title [draft] Fix #3365 ES preserveModules barrel export alias indexing fix(deconflictChunk): ES preserveModules barrel export alias indexing Mar 10, 2020
@kamranayub kamranayub changed the title fix(deconflictChunk): ES preserveModules barrel export alias indexing fix(deconflictChunk): Deconflict multiple index imports for ES format using nested export star statements Mar 10, 2020
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

See my comment

@kamranayub
Copy link
Contributor Author

kamranayub commented Mar 11, 2020

Thanks @lukastaegert, updated per your guidance.

Implications for consumers that I see right now:

  • Slightly increased bundle sizes due to the extra characters generated
  • Impact to code that might be diving into generated output expecting non-deconflicted variable names (e.g. other -> other$1)

Copy link
Member

@lukastaegert lukastaegert left a 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);
}
Copy link
Member

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 👍

@lukastaegert lukastaegert merged commit ff72e90 into rollup:master Mar 12, 2020
@kamranayub kamranayub deleted the fix/gh-3365/preserve-modules-export branch March 12, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate import identifier when re-exporting with export star when preserveModules is true
2 participants