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 for id$exports being undefined #9331

Merged
merged 14 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
78 changes: 58 additions & 20 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,15 @@ export default class BundleGraph {
// Disable in dev mode because this feature is at odds with safeToIncrementallyBundle
isProduction
) {
let nodeValueSymbols = node.value.symbols;

let source;
if (node.value.sourceAssetId != null) {
source = graph.getNodeByContentKey(
nullthrows(node.value.sourceAssetId),
);
}

// asset -> symbols that should be imported directly from that asset
let targets = new DefaultMap<ContentKey, Map<Symbol, Symbol>>(
() => new Map(),
Expand Down Expand Up @@ -252,6 +261,16 @@ export default class BundleGraph {
([, t]) => new Set([...t.values()]).size === t.size,
)
) {
let sourceAssetSymbols;
if (source) {
invariant(source.type === 'asset');
sourceAssetSymbols = nullthrows(source.value.symbols);
}
let isReexportAll = nodeValueSymbols.get('*')?.local === '*';
let reexportAllLoc = isReexportAll
? nullthrows(nodeValueSymbols.get('*')).loc
: undefined;

// TODO adjust sourceAssetIdNode.value.dependencies ?
let deps = [
// Keep the original dependency
Expand All @@ -261,13 +280,11 @@ export default class BundleGraph {
...node,
value: {
...node.value,
symbols: node.value.symbols
? new Map(
[...node.value.symbols].filter(([k]) =>
externalSymbols.has(k),
),
)
: undefined,
symbols: new Map(
[...nodeValueSymbols].filter(([k]) =>
externalSymbols.has(k),
),
),
},
usedSymbolsUp: new Map(
[...node.usedSymbolsUp].filter(([k]) =>
Expand All @@ -282,6 +299,34 @@ export default class BundleGraph {
let newNodeId = hashString(
node.id + [...target.keys()].join(','),
);

let symbols = new Map();
for (let [as, from] of target) {
let existing = nodeValueSymbols.get(as);
if (existing) {
symbols.set(from, existing);
} else {
invariant(isReexportAll);
let local = `${node.value.id}$rewrite$${asset}$${from}`;
symbols.set(from, {
isWeak: true,
local,
loc: reexportAllLoc,
});
// It might already exist with multiple export-alls causing ambiguous resolution
if (sourceAssetSymbols && !sourceAssetSymbols.has(as)) {
sourceAssetSymbols.set(as, {
thebriando marked this conversation as resolved.
Show resolved Hide resolved
loc: reexportAllLoc,
local: local,
});
}
}
}
let usedSymbolsUp = new Map(
[...node.usedSymbolsUp]
.filter(([k]) => target.has(k) || k === '*')
.map(([k, v]) => [target.get(k) ?? k, v]),
);
return {
asset,
dep: graph.addNodeByContentKey(newNodeId, {
Expand All @@ -290,24 +335,17 @@ export default class BundleGraph {
value: {
...node.value,
id: newNodeId,
symbols: node.value.symbols
? new Map(
[...node.value.symbols]
.filter(([k]) => target.has(k) || k === '*')
.map(([k, v]) => [target.get(k) ?? k, v]),
)
: undefined,
symbols,
},
usedSymbolsUp: new Map(
[...node.usedSymbolsUp]
.filter(([k]) => target.has(k) || k === '*')
.map(([k, v]) => [target.get(k) ?? k, v]),
),
usedSymbolsUp,
// This is only a temporary helper needed during symbol propagation and is never
// read afterwards.
usedSymbolsDown: new Set(),
}),
};
}),
];

dependencies.set(nodeId, deps);

// Jump to the dependencies that are used in this dependency
Expand Down Expand Up @@ -374,7 +412,6 @@ export default class BundleGraph {
);
}
}

return new BundleGraph({
graph,
assetPublicIds,
Expand Down Expand Up @@ -1777,6 +1814,7 @@ export default class BundleGraph {
}
}
}

// We didn't find the exact symbol...
if (potentialResults.length == 1) {
// ..., but if it does exist, it has to be behind this one reexport.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import * as foo from "./library/a.js";
output = foo.bar();
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./b.js";
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as bar } from "./c.js";
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function () {
return 2;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"sideEffects": [
"a.js"
]
}
19 changes: 19 additions & 0 deletions packages/core/integration-tests/test/scope-hoisting.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ describe('scope hoisting', function () {
let output = await run(b);
assert.equal(output, 2);
});

it('supports named exports of variables with a different name when wrapped', async function () {
let b = await bundle(
path.join(
Expand All @@ -147,6 +148,24 @@ describe('scope hoisting', function () {
assert.equal(output, 2);
});

it('supports import * as from a library that has export *', async function () {
let b = await bundle(
path.join(
__dirname,
'integration/scope-hoisting/es6/rewrite-export-star/index.js',
),
);
let output = await run(b);
assert.equal(output, 2);

assert.deepStrictEqual(
new Set(
b.getUsedSymbols(findDependency(b, 'index.js', './library/a.js')),
),
new Set(['bar']),
);
});

it('supports renaming non-ASCII identifiers', async function () {
let b = await bundle(
path.join(
Expand Down
Loading