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

Don't retarget dependencies if a symbol is imported multiple times with different local names #8738

Merged
merged 2 commits into from
Jan 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,12 @@ export default class BundleGraph {
}

if (
// Only perform rewriting when there is an imported symbol
// Only perform retargeting when there is an imported symbol
// - If the target is side-effect-free, the symbols point to the actual target and removing
// the original dependency resolution is fine
// - Otherwise, keep this dependency unchanged for its potential side effects
node.usedSymbolsUp.size > 0 &&
// Only perform rewriting if the dependency only points to a single asset (e.g. CSS modules)
// Only perform retargeting if the dependency only points to a single asset (e.g. CSS modules)
!hasAmbiguousSymbols &&
// It doesn't make sense to retarget dependencies where `*` is used, because the
// retargeting won't enable any benefits in that case (apart from potentially even more
Expand All @@ -243,7 +243,13 @@ export default class BundleGraph {
// or
// (parcelRequire("...")).then((a)=>a);
// if the reexporting asset did `export {a as b}` or `export * as a`
node.value.priority === Priority.sync
node.value.priority === Priority.sync &&
// For every asset, no symbol is imported multiple times (with a different local name).
// Don't retarget because this cannot be resolved without also changing the asset symbols
// (and the asset content itself).
[...targets].every(
([, t]) => new Set([...t.values()]).size === t.size,
)
) {
// TODO adjust sourceAssetIdNode.value.dependencies ?
let deps = [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import * as ns from "./library/a";

output = [ns, ns.value1, ns.value2];
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { value1, value2 } from './b.js';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { value1, value1 as value2 } from "./c";
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const value1 = 123;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"sideEffects": false
}
39 changes: 26 additions & 13 deletions packages/core/integration-tests/test/javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -7079,19 +7079,6 @@ describe('javascript', function () {
assert.deepEqual(res.output, 'bar');
});

it('supports reexports via variable declaration (unused)', async function () {
let b = await bundle(
path.join(
__dirname,
'/integration/scope-hoisting/es6/side-effects-re-exports-rename-var-unused/index.js',
),
options,
);

let res = await run(b, {}, {require: false});
assert.deepEqual((await res.output).foo, 'foo');
});

it('supports named and renamed reexports of the same asset (named used)', async function () {
let b = await bundle(
path.join(
Expand Down Expand Up @@ -7126,6 +7113,32 @@ describe('javascript', function () {
assert.deepEqual(res.output, 'bar');
});

it('supports named and renamed reexports of the same asset (namespace used)', async function () {
let b = await bundle(
path.join(
__dirname,
'/integration/scope-hoisting/es6/side-effects-re-exports-rename-same/index.js',
),
options,
);

let res = await run(b, null, {require: false});
assert.deepEqual(res.output, [{value1: 123, value2: 123}, 123, 123]);
});

it('supports reexports via variable declaration (unused)', async function () {
let b = await bundle(
path.join(
__dirname,
'/integration/scope-hoisting/es6/side-effects-re-exports-rename-var-unused/index.js',
),
options,
);

let res = await run(b, {}, {require: false});
assert.deepEqual((await res.output).foo, 'foo');
});

it('supports named and namespace exports of the same asset (named used)', async function () {
let b = await bundle(
path.join(
Expand Down