Fix: handle re-exports of synthetic named exports #3319

manucorporat · 2020-01-05T16:22:27Z

This PR contains:

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:

Description

Fixes issues with synthetic named exports when using them cross-chunk and (re-)exporting them. TODO:

Re-exported synthetic named exports need to be included in generated namespace objects
Check handling of external files

manucorporat · 2020-01-05T16:23:30Z

@lukastaegert systemjs finalizer might be a little trickier, any tip?

codecov · 2020-01-05T16:49:05Z

Codecov Report

Merging #3319 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            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

Impacted Files	Coverage Δ
src/Chunk.ts	`99.78% <100.00%> (+<0.01%)`	⬆️
src/Module.ts	`98.87% <100.00%> (+0.02%)`	⬆️
src/ast/variables/NamespaceVariable.ts	`100.00% <100.00%> (ø)`
src/ast/variables/SyntheticNamedExportVariable.ts	`92.30% <100.00%> (+4.80%)`	⬆️
src/ast/variables/Variable.ts	`95.83% <100.00%> (-0.33%)`	⬇️
src/finalisers/es.ts	`100.00% <100.00%> (ø)`
src/finalisers/system.ts	`96.62% <100.00%> (+0.15%)`	⬆️
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 d18cb37...1b81c71. Read the comment docs.

manucorporat · 2020-01-05T17:04:06Z

Just noticed it's not working great with multiple entry-points, working in a fix

lukastaegert

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.

lukastaegert · 2020-01-06T19:32:31Z

src/Chunk.ts

@@ -63,6 +64,7 @@ export interface ModuleDeclarationDependency {
 export type ChunkDependencies = ModuleDeclarationDependency[];

 export type ChunkExports = {
+	auxLocal: string | null;


What does auxLocal stand for? Can we find a better name so that it is clearer what it does?

lukastaegert · 2020-01-06T20:06:08Z

src/ast/variables/SyntheticNamedExportVariable.ts

+	getName(): string {
+		const name = this.name;
+		const renderBaseName = this.defaultVariable.getName();
+		console.log(this.defaultVariable.getOriginalVariable());


log statement

src/Chunk.ts

lukastaegert · 2020-01-06T20:30:46Z

src/finalisers/esm.ts

 		} else {
+			if (specifier.auxLocal) {
+				exportBlock.push(`${varOrConst} ${specifier.auxLocal}${_}=${_}${local};`);
+				local = specifier.auxLocal;


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.

lukastaegert · 2020-01-06T20:36:26Z

test/chunking-form/samples/mixed-synthetic-named-exports-2/_expected/es/generated-dep2.js

@@ -0,0 +1,3 @@
+var dep2 = {bar: {foo: 'works'}};
+
+export default dep2;


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.

you right! I just noticed rollup never emits default exports in generated chunks

lukastaegert · 2020-01-06T20:41:38Z

src/Chunk.ts

 			exports.push({
+				auxLocal,


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.

manucorporat · 2020-01-06T22:11:35Z

@lukastaegert applied some of your feedback, still a couple of things pending.
But added some code to prevent conflict, not sure if it's correct though. seems like it's a little hacky where i added it

manucorporat · 2020-01-06T22:13:31Z

src/Chunk.ts

@@ -1131,11 +1157,13 @@ export default class Chunk {
 					exportVariable.setRenderNames('exports', exportName);
 				} else {
 					exportVariable.setRenderNames(null, null);
+					if (options.format === 'es' && exportVariable instanceof SyntheticNamedExportVariable) {


needs a comment explaining why:

// ESM exports of synthetic events will render a new variable, here we make sure it's deconflicted

lukastaegert · 2020-01-07T12:55:08Z

src/Chunk.ts

-					variable instanceof ExportDefaultVariable ? variable.getOriginalVariable() : variable;
+					variable instanceof ExportDefaultVariable ||
+					variable instanceof SyntheticNamedExportVariable
+						? variable.getOriginalVariable()


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)

lukastaegert · 2020-01-07T12:57:09Z

src/Chunk.ts

-								: variable.module!.chunk!.getVariableExportName(variable),
-						local: variable.getName()
-					});
+					if (variable instanceof SyntheticNamedExportVariable) {


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.

src/Chunk.ts

src/finalisers/esm.ts

lukastaegert · 2020-01-09T17:23:11Z

Hi, I have some time tomorrow where I could work on this if you want, but just an offer as it is your PR.

LarsDenBakker · 2020-01-19T16:14:01Z

@manucorporat anything that can be done to help move this along? :)

manucorporat · 2020-03-31T12:15:47Z

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

shellscape · 2020-03-31T15:27:00Z

@lukastaegert @LarsDenBakker please do attack this

@manucorporat no worries, we understand!

lukastaegert · 2020-04-05T18:30:58Z

I will try to tackle this one now.

… and default imports

lukastaegert · 2020-04-06T21:33:48Z

Actually had some progress here, reexports are now working in most cases. Remaining things to check and/or fix that I am aware of:

Generated namespace objects omit re-exported synthetic named exports at the moment
Handling of external files with synthetic named exports should be investigated

Will try to fix them in the next days.

lukastaegert · 2020-04-07T20:46:13Z

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.

manucorporat mentioned this pull request Jan 5, 2020

use syntheticNamedExports rollup/plugins#142

Closed

manucorporat marked this pull request as ready for review January 5, 2020 16:45

lukastaegert reviewed Jan 6, 2020

View reviewed changes

manucorporat commented Jan 6, 2020

View reviewed changes

lukastaegert reviewed Jan 7, 2020

View reviewed changes

LarsDenBakker mentioned this pull request Jan 8, 2020

feat(commonjs): set syntheticNamedExports for commonjs modules rollup/plugins#149

Merged

9 tasks

lukastaegert self-assigned this Apr 5, 2020

manucorporat and others added 11 commits April 5, 2020 21:17

Fix: handle re-exports of synthetic named exports

11b6ad6

fix systemjs

85eae4f

reenable all tests

8009cc6

emit var or const

5127bb6

fix reexports

ec7247e

apply feedback

75aa3b6

wip

440c35b

Add test case for global name conflict

b18e078

Add test case for multi-level synthetic named exports

ed641f7

Add test case for handling and deduplicating multiple synthetic named…

b6a030b

… and default imports

Fix esm synthetic export deconflicting

023efd6

lukastaegert force-pushed the fix-export-synthetic branch from 3124824 to 023efd6 Compare April 6, 2020 14:59

lukastaegert added 2 commits April 6, 2020 22:57

Fix reeports of synthetic named exports

16e6718

Slightly improve coverage

9efe776

Fix namespace object generation

1b81c71

lukastaegert approved these changes Apr 7, 2020

View reviewed changes

lukastaegert merged commit aecd3e1 into rollup:master Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: handle re-exports of synthetic named exports #3319

Fix: handle re-exports of synthetic named exports #3319

manucorporat commented Jan 5, 2020 •

edited by lukastaegert

manucorporat commented Jan 5, 2020 •

edited

codecov bot commented Jan 5, 2020 •

edited

manucorporat commented Jan 5, 2020

lukastaegert left a comment

lukastaegert Jan 6, 2020

lukastaegert Jan 6, 2020

lukastaegert Jan 6, 2020

lukastaegert Jan 6, 2020

manucorporat Jan 6, 2020

lukastaegert Jan 6, 2020

manucorporat commented Jan 6, 2020

manucorporat Jan 6, 2020

lukastaegert Jan 7, 2020

lukastaegert Jan 7, 2020

lukastaegert commented Jan 9, 2020

LarsDenBakker commented Jan 19, 2020

manucorporat commented Mar 31, 2020

shellscape commented Mar 31, 2020

lukastaegert commented Apr 5, 2020

lukastaegert commented Apr 6, 2020

lukastaegert commented Apr 7, 2020

		@@ -0,0 +1,3 @@
		var dep2 = {bar: {foo: 'works'}};

		export default dep2;

Fix: handle re-exports of synthetic named exports #3319

Fix: handle re-exports of synthetic named exports #3319

Conversation

manucorporat commented Jan 5, 2020 • edited by lukastaegert

Description

manucorporat commented Jan 5, 2020 • edited

codecov bot commented Jan 5, 2020 • edited

Codecov Report

manucorporat commented Jan 5, 2020

lukastaegert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manucorporat commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Jan 9, 2020

LarsDenBakker commented Jan 19, 2020

manucorporat commented Mar 31, 2020

shellscape commented Mar 31, 2020

lukastaegert commented Apr 5, 2020

lukastaegert commented Apr 6, 2020

lukastaegert commented Apr 7, 2020

manucorporat commented Jan 5, 2020 •

edited by lukastaegert

manucorporat commented Jan 5, 2020 •

edited

codecov bot commented Jan 5, 2020 •

edited