From febe169948376cceb60efd9d5a5982106dc80b5a Mon Sep 17 00:00:00 2001 From: Brian Do Date: Tue, 7 Nov 2023 13:04:12 -0800 Subject: [PATCH] Fix for id$exports being undefined (#9331) * 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> --- packages/core/core/src/BundleGraph.js | 90 ++++++++++++++----- .../es6/rewrite-export-star/index.js | 2 + .../es6/rewrite-export-star/library/a.js | 1 + .../es6/rewrite-export-star/library/b.js | 1 + .../es6/rewrite-export-star/library/c.js | 3 + .../rewrite-export-star/library/package.json | 5 ++ .../integration-tests/test/scope-hoisting.js | 20 +++++ 7 files changed, 101 insertions(+), 21 deletions(-) create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/index.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/a.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/b.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/c.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/package.json diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index c70b582a977..c2fdf22062c 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -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>( () => new Map(), @@ -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 @@ -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]) => @@ -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, { @@ -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 @@ -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); } @@ -374,7 +422,6 @@ export default class BundleGraph { ); } } - return new BundleGraph({ graph, assetPublicIds, @@ -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. diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/index.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/index.js new file mode 100644 index 00000000000..9415a1eca3a --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/index.js @@ -0,0 +1,2 @@ +import * as foo from "./library/a.js"; +output = foo.bar(); diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/a.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/a.js new file mode 100644 index 00000000000..d165e999022 --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/a.js @@ -0,0 +1 @@ +export * from "./b.js"; diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/b.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/b.js new file mode 100644 index 00000000000..22fb309d02d --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/b.js @@ -0,0 +1 @@ +export { default as bar } from "./c.js"; diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/c.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/c.js new file mode 100644 index 00000000000..db5a3c674ce --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/c.js @@ -0,0 +1,3 @@ +export default function () { + return 2; +} diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/package.json b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/package.json new file mode 100644 index 00000000000..c523e5cd63c --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/rewrite-export-star/library/package.json @@ -0,0 +1,5 @@ +{ + "sideEffects": [ + "a.js" + ] +} diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index 348e93648ce..26a64eb4db4 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -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( @@ -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(