Skip to content

Commit

Permalink
Fix for id$exports being undefined (#9331)
Browse files Browse the repository at this point in the history
* WIP

* Check if source is in graph before looking at its symbols

* Cleanup logs

* Add test

* Bundle in prod mode in test

* Don't mutate asset graph

* Check if source asset exists before changing symbols

---------

Co-authored-by: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com>
  • Loading branch information
thebriando and mischnic authored Nov 7, 2023
1 parent 817c778 commit febe169
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 21 deletions.
90 changes: 69 additions & 21 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ export default class BundleGraph {
// Disable in dev mode because this feature is at odds with safeToIncrementallyBundle
isProduction
) {
let nodeValueSymbols = node.value.symbols;

// 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 +254,11 @@ export default class BundleGraph {
([, t]) => new Set([...t.values()]).size === t.size,
)
) {
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 +268,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 +287,49 @@ 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 (
node.value.sourceAssetId != null &&
assetGraph.hasContentKey(node.value.sourceAssetId)
) {
let sourceAssetId = nullthrows(
assetGraphNodeIdToBundleGraphNodeId.get(
assetGraph.getNodeIdByContentKey(
nullthrows(node.value.sourceAssetId),
),
),
);
let sourceAsset = nullthrows(graph.getNode(sourceAssetId));
invariant(sourceAsset.type === 'asset');
let sourceAssetSymbols = sourceAsset.value.symbols;
if (sourceAssetSymbols && !sourceAssetSymbols.has(as)) {
sourceAssetSymbols.set(as, {
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 +338,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 All @@ -323,7 +364,14 @@ export default class BundleGraph {
}
// Don't copy over asset groups into the bundle graph.
else if (node.type !== 'asset_group') {
let bundleGraphNodeId = graph.addNodeByContentKey(node.id, node);
let nodeToAdd =
node.type === 'asset'
? {
...node,
value: {...node.value, symbols: new Map(node.value.symbols)},
}
: node;
let bundleGraphNodeId = graph.addNodeByContentKey(node.id, nodeToAdd);
if (node.id === assetGraphRootNode?.id) {
graph.setRootNodeId(bundleGraphNodeId);
}
Expand Down Expand Up @@ -374,7 +422,6 @@ export default class BundleGraph {
);
}
}

return new BundleGraph({
graph,
assetPublicIds,
Expand Down Expand Up @@ -1781,6 +1828,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"
]
}
20 changes: 20 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,25 @@ 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',
),
{mode: 'production'},
);
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

0 comments on commit febe169

Please sign in to comment.