-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: handle re-exports of synthetic named exports #3319
Fix: handle re-exports of synthetic named exports #3319
Conversation
@lukastaegert systemjs finalizer might be a little trickier, any tip? |
Codecov Report
@@ Coverage Diff @@
## master #3319 +/- ##
==========================================
+ Coverage 95.92% 95.94% +0.02%
==========================================
Files 174 174
Lines 5911 5950 +39
Branches 1739 1752 +13
==========================================
+ Hits 5670 5709 +39
Misses 124 124
Partials 117 117
Continue to review full report at Codecov.
|
Just noticed it's not working great with multiple entry-points, working in a fix |
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 see this is still WIP, still some comments, unfortunately probably not too helpful ones, including a broken test case. The question of variable name conflicts is also a tough one.
src/Chunk.ts
Outdated
@@ -63,6 +64,7 @@ export interface ModuleDeclarationDependency { | |||
export type ChunkDependencies = ModuleDeclarationDependency[]; | |||
|
|||
export type ChunkExports = { | |||
auxLocal: string | null; |
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 does auxLocal
stand for? Can we find a better name so that it is clearer what it does?
getName(): string { | ||
const name = this.name; | ||
const renderBaseName = this.defaultVariable.getName(); | ||
console.log(this.defaultVariable.getOriginalVariable()); |
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.
log statement
src/finalisers/esm.ts
Outdated
} else { | ||
if (specifier.auxLocal) { | ||
exportBlock.push(`${varOrConst} ${specifier.auxLocal}${_}=${_}${local};`); | ||
local = specifier.auxLocal; |
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.
Is there any logic that makes sure that this new variable does not conflict with an existing one? Worst case, it could conflict with an accessed global variable, in which case the used specifier CANNOT be identical with the property name. In all other cases, it would still need to be deconflicted with all imports and other top-level variables. Unfortunately I do not see a trivial solution for this yet.
@@ -0,0 +1,3 @@ | |||
var dep2 = {bar: {foo: 'works'}}; | |||
|
|||
export default dep2; |
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 do not think this should be rendered as a default export. Usually, there are no default exports in auto-generated chunks and everything is a named export. Furthermore, the named exports are also minified. I think we definitely need this here as well, the most important reason being that a file can have only one default export while a chunk could potentially have many of those synthetic exports belonging to different default exports. I think this bug is also shown in the test case I added in the other comment.
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.
you right! I just noticed rollup never emits default exports in generated chunks
src/Chunk.ts
Outdated
exports.push({ | ||
auxLocal, |
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.
Maybe it is a property
? I really find auxLocal
unhelpful as it does not tell me what this value means. property
could tell me that this is the property we are exporting. The fact that I need to introduce a local variable is of course another question.
@lukastaegert applied some of your feedback, still a couple of things pending. |
src/Chunk.ts
Outdated
@@ -1131,11 +1157,13 @@ export default class Chunk { | |||
exportVariable.setRenderNames('exports', exportName); | |||
} else { | |||
exportVariable.setRenderNames(null, null); | |||
if (options.format === 'es' && exportVariable instanceof SyntheticNamedExportVariable) { |
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.
needs a comment explaining why:
// ESM exports of synthetic events will render a new variable, here we make sure it's deconflicted
src/Chunk.ts
Outdated
variable instanceof ExportDefaultVariable ? variable.getOriginalVariable() : variable; | ||
variable instanceof ExportDefaultVariable || | ||
variable instanceof SyntheticNamedExportVariable | ||
? variable.getOriginalVariable() |
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.
At the moment, this will only go one level deep. It would be nice if when a default export is itself a synthetic named export, only the original default export is ever passed between chunks. I created a broken test case for this that currently produces wild results (synthetic-named-exports-multi-level)
src/Chunk.ts
Outdated
: variable.module!.chunk!.getVariableExportName(variable), | ||
local: variable.getName() | ||
}); | ||
if (variable instanceof SyntheticNamedExportVariable) { |
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.
So what we did together with the code in line 964 is that we replaced variable with variable.defaultVariable if it was a synthetic named export and now we basically do it a second time if the default itself was synthetic. What it should do, however, is work for arbitrary levels of nesting, see one of the test cases I added.
Hi, I have some time tomorrow where I could work on this if you want, but just an offer as it is your PR. |
@manucorporat anything that can be done to help move this along? :) |
Unfortunately, it's being hard to find time to keep working in this PR from a mental and working perspective, feel free anyone to keep working on it. Otherwise since this is not high priority i will try to find sometime this summer! Sorry |
@lukastaegert @LarsDenBakker please do attack this @manucorporat no worries, we understand! |
I will try to tackle this one now. |
… and default imports
3124824
to
023efd6
Compare
Actually had some progress here, reexports are now working in most cases. Remaining things to check and/or fix that I am aware of:
Will try to fix them in the next days. |
From my side, this is now ready for release! Quite a lot of stuff has been fixed. I will have a last look through the code and push a patch release so that the CJS plugin can move forward. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Fixes issues with synthetic named exports when using them cross-chunk and (re-)exporting them. TODO: