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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Mar 12, 2020
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ac4af85
Add failing test for barrel exports #3365
kamranayub Mar 10, 2020
a96f344
Update expected outputs
kamranayub Mar 10, 2020
a1e404c
Fix expected output
kamranayub Mar 10, 2020
58d6d89
Set chunkOrExternalModule.variableName in deconflictImportsEsm
kamranayub Mar 10, 2020
53b45c4
Dconflict deps when preserveModules is true
kamranayub Mar 11, 2020
4a8dbe8
Update system-exports-wrap-pure-annotation
kamranayub Mar 11, 2020
15cf5ab
Actually use preserveModules flag
kamranayub Mar 11, 2020
b804496
Only deconflict deps in esm to minimize impact
kamranayub Mar 11, 2020
04a4385
Update tests expected output
kamranayub Mar 11, 2020
3e97dee
Update test, add more context to comment
kamranayub Mar 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/utils/deconflictChunk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,18 @@ export function deconflictChunk(
function deconflictImportsEsm(
usedNames: Set<string>,
imports: Set<Variable>,
_dependencies: Set<ExternalModule | Chunk>,
dependencies: Set<ExternalModule | Chunk>,
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).

// only deconflict index aliased variables for ESM/System
if (chunkOrExternalModule.variableName === 'index') {
chunkOrExternalModule.variableName = getSafeName(
chunkOrExternalModule.variableName,
usedNames
);
}
}
for (const variable of imports) {
const module = variable.module;
const name = variable.name;
Expand Down
7 changes: 7 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module.exports = {
description: 'confirm exports are deconflicted when exporting nested index aliases',
options: {
input: 'main.js',
preserveModules: true
}
};
10 changes: 10 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/_expected/amd/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
define(['exports', './module-a/v1/index', './module-b/v1/index'], function (exports, index, index$1) { 'use strict';



exports.ModuleA_V1 = index;
exports.ModuleB_V1 = index$1;

Object.defineProperty(exports, '__esModule', { value: true });

});
9 changes: 9 additions & 0 deletions ...-form/samples/preserve-modules-nested-barrel-es-export/_expected/amd/module-a/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
define(['exports'], function (exports) { 'use strict';

const TEST_MODULE_A = 'A';

exports.TEST_MODULE_A = TEST_MODULE_A;

Object.defineProperty(exports, '__esModule', { value: true });

});
9 changes: 9 additions & 0 deletions ...-form/samples/preserve-modules-nested-barrel-es-export/_expected/amd/module-b/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
define(['exports'], function (exports) { 'use strict';

const TEST_MODULE_B = 'A';

exports.TEST_MODULE_B = TEST_MODULE_B;

Object.defineProperty(exports, '__esModule', { value: true });

});
11 changes: 11 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/_expected/cjs/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
'use strict';

Object.defineProperty(exports, '__esModule', { value: true });

var index = require('./module-a/v1/index.js');
var index$1 = require('./module-b/v1/index.js');



exports.ModuleA_V1 = index;
exports.ModuleB_V1 = index$1;
7 changes: 7 additions & 0 deletions ...-form/samples/preserve-modules-nested-barrel-es-export/_expected/cjs/module-a/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

Object.defineProperty(exports, '__esModule', { value: true });

const TEST_MODULE_A = 'A';

exports.TEST_MODULE_A = TEST_MODULE_A;
7 changes: 7 additions & 0 deletions ...-form/samples/preserve-modules-nested-barrel-es-export/_expected/cjs/module-b/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

Object.defineProperty(exports, '__esModule', { value: true });

const TEST_MODULE_B = 'A';

exports.TEST_MODULE_B = TEST_MODULE_B;
4 changes: 4 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/_expected/es/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

export { index$1 as ModuleB_V1 };
3 changes: 3 additions & 0 deletions ...g-form/samples/preserve-modules-nested-barrel-es-export/_expected/es/module-a/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const TEST_MODULE_A = 'A';

export { TEST_MODULE_A };
3 changes: 3 additions & 0 deletions ...g-form/samples/preserve-modules-nested-barrel-es-export/_expected/es/module-b/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const TEST_MODULE_B = 'A';

export { TEST_MODULE_B };
15 changes: 15 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/_expected/system/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
System.register(['./module-a/v1/index.js', './module-b/v1/index.js'], function (exports) {
'use strict';
return {
setters: [function (module) {
exports('ModuleA_V1', module);
}, function (module) {
exports('ModuleB_V1', module);
}],
execute: function () {



}
};
});
10 changes: 10 additions & 0 deletions ...rm/samples/preserve-modules-nested-barrel-es-export/_expected/system/module-a/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
System.register([], function (exports) {
'use strict';
return {
execute: function () {

const TEST_MODULE_A = exports('TEST_MODULE_A', 'A');

}
};
});
10 changes: 10 additions & 0 deletions ...rm/samples/preserve-modules-nested-barrel-es-export/_expected/system/module-b/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
System.register([], function (exports) {
'use strict';
return {
execute: function () {

const TEST_MODULE_B = exports('TEST_MODULE_B', 'A');

}
};
});
2 changes: 2 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './module-a/index'
export * from './module-b/index'
3 changes: 3 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/module-a/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import * as ModuleA_V1 from './v1/index'

export { ModuleA_V1 }
1 change: 1 addition & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/module-a/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const TEST_MODULE_A = 'A'
3 changes: 3 additions & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/module-b/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import * as ModuleB_V1 from './v1/index'

export { ModuleB_V1 }
1 change: 1 addition & 0 deletions test/chunking-form/samples/preserve-modules-nested-barrel-es-export/module-b/v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const TEST_MODULE_B = 'A'