From a8345f796c8783776a9667ee69d9ed379f63349d Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Wed, 12 Jul 2023 09:07:06 +0200 Subject: [PATCH 1/7] WIP --- packages/core/core/src/BundleGraph.js | 156 ++++++++++++++---- .../packagers/js/src/ScopeHoistingPackager.js | 4 + packages/transformers/js/src/JSTransformer.js | 1 + 3 files changed, 126 insertions(+), 35 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index a5af4ecf59e..048d11d141a 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -42,6 +42,7 @@ import {getBundleGroupId, getPublicId} from './utils'; import {ISOLATED_ENVS} from './public/Environment'; import {fromProjectPath, fromProjectPathRelative} from './projectPath'; import {HASH_REF_PREFIX} from './constants'; +import inspect from 'graphql/jsutils/inspect'; export const bundleGraphEdgeTypes = { // A lack of an edge type indicates to follow the edge while traversing @@ -205,6 +206,13 @@ export default class BundleGraph { // Disable in dev mode because this feature is at odds with safeToIncrementallyBundle isProduction ) { + let nodeValueSymbols = node.value.symbols; + + let source = nullthrows( + graph.getNodeByContentKey(nullthrows(node.value.sourceAssetId)), + ); + invariant(source.type === 'asset'); + // asset -> symbols that should be imported directly from that asset let targets = new DefaultMap>( () => new Map(), @@ -252,36 +260,88 @@ export default class BundleGraph { ([, t]) => new Set([...t.values()]).size === t.size, ) ) { + console.log( + node.value.specifier, + [...targets].map(([a, s]) => [ + assetGraph.getNodeByContentKey(a)?.value.filePath, + s, + ]), + ); + + let 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 - { - asset: null, - dep: graph.addNodeByContentKey(node.id, { - ...node, - value: { - ...node.value, - symbols: node.value.symbols - ? new Map( - [...node.value.symbols].filter(([k]) => - externalSymbols.has(k), - ), - ) - : undefined, - }, - usedSymbolsUp: new Map( - [...node.usedSymbolsUp].filter(([k]) => - externalSymbols.has(k), - ), - ), - usedSymbolsDown: new Set(), - excluded: externalSymbols.size === 0, - }), - }, + // { + // asset: null, + // dep: graph.addNodeByContentKey(node.id, { + // ...node, + // value: { + // ...node.value, + // symbols: new Map( + // [...nodeValueSymbols].filter(([k]) => + // externalSymbols.has(k), + // ), + // ), + // }, + // usedSymbolsUp: new Map( + // [...node.usedSymbolsUp].filter(([k]) => + // externalSymbols.has(k), + // ), + // ), + // usedSymbolsDown: new Set(), + // excluded: externalSymbols.size === 0, + // }), + // }, ...[...targets].map(([asset, target]) => { let newNodeId = hashString( node.id + [...target.keys()].join(','), ); + + let symbols = new Map(); + console.log( + 'X', + require('util').inspect(nodeValueSymbols, {depth: Infinity}), + target, + isReexportAll, + ); + 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, + }); + console.log( + 'adding ', + as, + source.value.filePath, + sourceAssetSymbols, + ); + // It might already exist with multiple export-alls causing ambiguous resolution + if (!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 +350,28 @@ 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(), }), }; }), ]; + console.log( + node.value.specifier, + require('util').inspect( + deps.map(x => [ + assetGraph.getNodeByContentKey(x.asset)?.value.filePath, + [...graph.getNode(x.dep).usedSymbolsUp], + [...graph.getNode(x.dep).value.symbols], + ]), + {depth: Infinity}, + ), + ); + dependencies.set(nodeId, deps); // Jump to the dependencies that are used in this dependency @@ -1643,6 +1707,15 @@ export default class BundleGraph { symbol: Symbol, boundary: ?Bundle, ): InternalSymbolResolution { + // let isX = String(asset.filePath) === 'lib.js' && symbol === 'Foo'; + // let isY = String(asset.filePath) === 'libFoo2.js' && symbol === 'Foo'; + // if (isX) { + // global.X = true; + // } + // try { + if (global.X) { + console.log('enter', asset.filePath, symbol); + } let assetOutside = boundary && !this.bundleHasAsset(boundary, asset); let identifier = asset.symbols?.get(symbol)?.local; @@ -1727,6 +1800,9 @@ export default class BundleGraph { continue; } let result = this.getSymbolResolution(resolved, symbol, boundary); + // if (global.X) { + // console.log('recursive', resolved.filePath, symbol, result); + // } // We found the symbol if (result.symbol != undefined) { @@ -1777,6 +1853,11 @@ export default class BundleGraph { } } } + + // if (global.X) { + // console.log(asset.filePath, symbol, potentialResults); + // } + // We didn't find the exact symbol... if (potentialResults.length == 1) { // ..., but if it does exist, it has to be behind this one reexport. @@ -1810,6 +1891,11 @@ export default class BundleGraph { loc: asset.symbols?.get(symbol)?.loc, }; } + // } finally { + // if (isX) { + // global.X = false; + // } + // } } getAssetById(contentKey: string): Asset { let node = this._graph.getNodeByContentKey(contentKey); diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index d12a2bac987..70aceaf83e2 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -856,6 +856,10 @@ ${code} symbol, } = this.bundleGraph.getSymbolResolution(resolved, imported, this.bundle); + // if (imported === 'Foo') { + // console.log({resolved, imported}, {resolvedAsset, exportSymbol, symbol}); + // } + if ( resolvedAsset.type !== 'js' || (dep && this.bundleGraph.isDependencySkipped(dep)) diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index b96cae8ba2a..170f0ffbfb5 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -932,6 +932,7 @@ export default (new Transformer({ } asset.type = 'js'; + // console.log(asset.filePath, compiledCode.toString()) asset.setBuffer(compiledCode); if (map) { From 8ac0ab0f1f9de9c823f96c35ad743b54183ab71a Mon Sep 17 00:00:00 2001 From: Brian Do Date: Mon, 16 Oct 2023 12:49:42 -0700 Subject: [PATCH 2/7] Check if source is in graph before looking at its symbols --- packages/core/core/src/BundleGraph.js | 83 ++++++++++++++------------- 1 file changed, 43 insertions(+), 40 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index 048d11d141a..1ad330c01f2 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -199,6 +199,7 @@ export default class BundleGraph { walkVisited.add(nodeId); let node = nullthrows(assetGraph.getNode(nodeId)); + if ( node.type === 'dependency' && node.value.symbols != null && @@ -208,10 +209,9 @@ export default class BundleGraph { ) { let nodeValueSymbols = node.value.symbols; - let source = nullthrows( - graph.getNodeByContentKey(nullthrows(node.value.sourceAssetId)), + let source = graph.getNodeByContentKey( + nullthrows(node.value.sourceAssetId), ); - invariant(source.type === 'asset'); // asset -> symbols that should be imported directly from that asset let targets = new DefaultMap>( @@ -260,15 +260,19 @@ export default class BundleGraph { ([, t]) => new Set([...t.values()]).size === t.size, ) ) { - console.log( - node.value.specifier, - [...targets].map(([a, s]) => [ - assetGraph.getNodeByContentKey(a)?.value.filePath, - s, - ]), - ); - - let sourceAssetSymbols = nullthrows(source.value.symbols); + // console.log( + // node.value.specifier, + // [...targets].map(([a, s]) => [ + // assetGraph.getNodeByContentKey(a)?.value.filePath, + // s, + // ]), + // ); + + 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 @@ -304,12 +308,12 @@ export default class BundleGraph { ); let symbols = new Map(); - console.log( - 'X', - require('util').inspect(nodeValueSymbols, {depth: Infinity}), - target, - isReexportAll, - ); + // console.log( + // 'X', + // require('util').inspect(nodeValueSymbols, {depth: Infinity}), + // target, + // isReexportAll, + // ); for (let [as, from] of target) { let existing = nodeValueSymbols.get(as); if (existing) { @@ -322,14 +326,14 @@ export default class BundleGraph { local, loc: reexportAllLoc, }); - console.log( - 'adding ', - as, - source.value.filePath, - sourceAssetSymbols, - ); + // console.log( + // 'adding ', + // as, + // source.value.filePath, + // sourceAssetSymbols, + // ); // It might already exist with multiple export-alls causing ambiguous resolution - if (!sourceAssetSymbols.has(as)) { + if (sourceAssetSymbols && !sourceAssetSymbols.has(as)) { sourceAssetSymbols.set(as, { loc: reexportAllLoc, local: local, @@ -360,17 +364,17 @@ export default class BundleGraph { }; }), ]; - console.log( - node.value.specifier, - require('util').inspect( - deps.map(x => [ - assetGraph.getNodeByContentKey(x.asset)?.value.filePath, - [...graph.getNode(x.dep).usedSymbolsUp], - [...graph.getNode(x.dep).value.symbols], - ]), - {depth: Infinity}, - ), - ); + // console.log( + // node.value.specifier, + // require('util').inspect( + // deps.map(x => [ + // assetGraph.getNodeByContentKey(x.asset)?.value.filePath, + // [...graph.getNode(x.dep).usedSymbolsUp], + // [...graph.getNode(x.dep).value.symbols], + // ]), + // {depth: Infinity}, + // ), + // ); dependencies.set(nodeId, deps); @@ -438,7 +442,6 @@ export default class BundleGraph { ); } } - return new BundleGraph({ graph, assetPublicIds, @@ -1713,9 +1716,9 @@ export default class BundleGraph { // global.X = true; // } // try { - if (global.X) { - console.log('enter', asset.filePath, symbol); - } + // if (global.X) { + // console.log('enter', asset.filePath, symbol); + // } let assetOutside = boundary && !this.bundleHasAsset(boundary, asset); let identifier = asset.symbols?.get(symbol)?.local; From 61675d9e180176ebfbf4274518b53b7a6b08eb50 Mon Sep 17 00:00:00 2001 From: Brian Do Date: Tue, 17 Oct 2023 15:02:17 -0700 Subject: [PATCH 3/7] Cleanup logs --- packages/core/core/src/BundleGraph.js | 96 ++++--------------- .../packagers/js/src/ScopeHoistingPackager.js | 4 - packages/transformers/js/src/JSTransformer.js | 1 - 3 files changed, 21 insertions(+), 80 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index 1ad330c01f2..e8faf632097 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -42,7 +42,6 @@ import {getBundleGroupId, getPublicId} from './utils'; import {ISOLATED_ENVS} from './public/Environment'; import {fromProjectPath, fromProjectPathRelative} from './projectPath'; import {HASH_REF_PREFIX} from './constants'; -import inspect from 'graphql/jsutils/inspect'; export const bundleGraphEdgeTypes = { // A lack of an edge type indicates to follow the edge while traversing @@ -199,7 +198,6 @@ export default class BundleGraph { walkVisited.add(nodeId); let node = nullthrows(assetGraph.getNode(nodeId)); - if ( node.type === 'dependency' && node.value.symbols != null && @@ -260,14 +258,6 @@ export default class BundleGraph { ([, t]) => new Set([...t.values()]).size === t.size, ) ) { - // console.log( - // node.value.specifier, - // [...targets].map(([a, s]) => [ - // assetGraph.getNodeByContentKey(a)?.value.filePath, - // s, - // ]), - // ); - let sourceAssetSymbols; if (source) { invariant(source.type === 'asset'); @@ -281,39 +271,33 @@ export default class BundleGraph { // TODO adjust sourceAssetIdNode.value.dependencies ? let deps = [ // Keep the original dependency - // { - // asset: null, - // dep: graph.addNodeByContentKey(node.id, { - // ...node, - // value: { - // ...node.value, - // symbols: new Map( - // [...nodeValueSymbols].filter(([k]) => - // externalSymbols.has(k), - // ), - // ), - // }, - // usedSymbolsUp: new Map( - // [...node.usedSymbolsUp].filter(([k]) => - // externalSymbols.has(k), - // ), - // ), - // usedSymbolsDown: new Set(), - // excluded: externalSymbols.size === 0, - // }), - // }, + { + asset: null, + dep: graph.addNodeByContentKey(node.id, { + ...node, + value: { + ...node.value, + symbols: new Map( + [...nodeValueSymbols].filter(([k]) => + externalSymbols.has(k), + ), + ), + }, + usedSymbolsUp: new Map( + [...node.usedSymbolsUp].filter(([k]) => + externalSymbols.has(k), + ), + ), + usedSymbolsDown: new Set(), + excluded: externalSymbols.size === 0, + }), + }, ...[...targets].map(([asset, target]) => { let newNodeId = hashString( node.id + [...target.keys()].join(','), ); let symbols = new Map(); - // console.log( - // 'X', - // require('util').inspect(nodeValueSymbols, {depth: Infinity}), - // target, - // isReexportAll, - // ); for (let [as, from] of target) { let existing = nodeValueSymbols.get(as); if (existing) { @@ -326,12 +310,6 @@ export default class BundleGraph { local, loc: reexportAllLoc, }); - // console.log( - // 'adding ', - // as, - // source.value.filePath, - // sourceAssetSymbols, - // ); // It might already exist with multiple export-alls causing ambiguous resolution if (sourceAssetSymbols && !sourceAssetSymbols.has(as)) { sourceAssetSymbols.set(as, { @@ -364,17 +342,6 @@ export default class BundleGraph { }; }), ]; - // console.log( - // node.value.specifier, - // require('util').inspect( - // deps.map(x => [ - // assetGraph.getNodeByContentKey(x.asset)?.value.filePath, - // [...graph.getNode(x.dep).usedSymbolsUp], - // [...graph.getNode(x.dep).value.symbols], - // ]), - // {depth: Infinity}, - // ), - // ); dependencies.set(nodeId, deps); @@ -1710,15 +1677,6 @@ export default class BundleGraph { symbol: Symbol, boundary: ?Bundle, ): InternalSymbolResolution { - // let isX = String(asset.filePath) === 'lib.js' && symbol === 'Foo'; - // let isY = String(asset.filePath) === 'libFoo2.js' && symbol === 'Foo'; - // if (isX) { - // global.X = true; - // } - // try { - // if (global.X) { - // console.log('enter', asset.filePath, symbol); - // } let assetOutside = boundary && !this.bundleHasAsset(boundary, asset); let identifier = asset.symbols?.get(symbol)?.local; @@ -1803,9 +1761,6 @@ export default class BundleGraph { continue; } let result = this.getSymbolResolution(resolved, symbol, boundary); - // if (global.X) { - // console.log('recursive', resolved.filePath, symbol, result); - // } // We found the symbol if (result.symbol != undefined) { @@ -1857,10 +1812,6 @@ export default class BundleGraph { } } - // if (global.X) { - // console.log(asset.filePath, symbol, potentialResults); - // } - // We didn't find the exact symbol... if (potentialResults.length == 1) { // ..., but if it does exist, it has to be behind this one reexport. @@ -1894,11 +1845,6 @@ export default class BundleGraph { loc: asset.symbols?.get(symbol)?.loc, }; } - // } finally { - // if (isX) { - // global.X = false; - // } - // } } getAssetById(contentKey: string): Asset { let node = this._graph.getNodeByContentKey(contentKey); diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 70aceaf83e2..d12a2bac987 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -856,10 +856,6 @@ ${code} symbol, } = this.bundleGraph.getSymbolResolution(resolved, imported, this.bundle); - // if (imported === 'Foo') { - // console.log({resolved, imported}, {resolvedAsset, exportSymbol, symbol}); - // } - if ( resolvedAsset.type !== 'js' || (dep && this.bundleGraph.isDependencySkipped(dep)) diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index 170f0ffbfb5..b96cae8ba2a 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -932,7 +932,6 @@ export default (new Transformer({ } asset.type = 'js'; - // console.log(asset.filePath, compiledCode.toString()) asset.setBuffer(compiledCode); if (map) { From 7cbcd0a8316313fc057f8dca4034e0d856482003 Mon Sep 17 00:00:00 2001 From: Brian Do Date: Wed, 18 Oct 2023 23:44:42 -0700 Subject: [PATCH 4/7] Add test --- packages/core/core/src/BundleGraph.js | 9 ++++++--- .../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 | 19 +++++++++++++++++++ 7 files changed, 37 insertions(+), 3 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 e8faf632097..868dfbdce36 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -207,9 +207,12 @@ export default class BundleGraph { ) { let nodeValueSymbols = node.value.symbols; - let source = graph.getNodeByContentKey( - nullthrows(node.value.sourceAssetId), - ); + 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>( 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 a23e7608444..c91af11f194 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -134,6 +134,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( @@ -146,6 +147,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( From b2e47eba7e29953ca3c9f2875804096986c62a94 Mon Sep 17 00:00:00 2001 From: Brian Do Date: Tue, 24 Oct 2023 17:57:58 -0700 Subject: [PATCH 5/7] Bundle in prod mode in test --- packages/core/integration-tests/test/scope-hoisting.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index c91af11f194..bb48f858ba9 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -153,6 +153,7 @@ describe('scope hoisting', function () { __dirname, 'integration/scope-hoisting/es6/rewrite-export-star/index.js', ), + {mode: 'production'}, ); let output = await run(b); assert.equal(output, 2); From 85be565ac178ad1ae3cbcc558ef1e93105f5a14f Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Thu, 26 Oct 2023 21:58:05 +0200 Subject: [PATCH 6/7] Don't mutate asset graph --- packages/core/core/src/BundleGraph.js | 32 ++++++++++++++++----------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index 868dfbdce36..b21a4b99f30 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -207,13 +207,6 @@ export default class BundleGraph { ) { 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>( () => new Map(), @@ -261,11 +254,6 @@ 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 @@ -314,6 +302,17 @@ export default class BundleGraph { loc: reexportAllLoc, }); // It might already exist with multiple export-alls causing ambiguous resolution + + 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, @@ -361,7 +360,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); } From 852538c8c08f67361333838a083bd06ff1029862 Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Thu, 26 Oct 2023 21:58:05 +0200 Subject: [PATCH 7/7] Check if source asset exists before changing symbols --- packages/core/core/src/BundleGraph.js | 34 +++++++++++++++------------ 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index b21a4b99f30..bd5d3a400db 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -302,22 +302,26 @@ export default class BundleGraph { loc: reexportAllLoc, }); // It might already exist with multiple export-alls causing ambiguous resolution - - let sourceAssetId = nullthrows( - assetGraphNodeIdToBundleGraphNodeId.get( - assetGraph.getNodeIdByContentKey( - nullthrows(node.value.sourceAssetId), + 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 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, + }); + } } } }