From 40bf1c2859304d5f076972ecc5543af99e440e6e Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 8 Jun 2023 13:11:26 -0700 Subject: [PATCH] Fix self references in CSS module JS assets (#9080) --- packages/core/core/src/AssetGraph.js | 4 +++ packages/core/core/src/public/Asset.js | 13 +++++++ .../integration-tests/test/css-modules.js | 34 +++++++++++++++++++ .../css-module-self-references/a/index.js | 3 ++ .../css-module-self-references/a/package.json | 5 +++ .../css-module-self-references/b/index.js | 3 ++ .../css-module-self-references/b/package.json | 5 +++ .../css-module-self-references/bar.module.css | 8 +++++ packages/core/types/index.js | 6 ++++ packages/packagers/css/src/CSSPackager.js | 1 + .../transformers/css/src/CSSTransformer.js | 26 ++++++++++---- packages/transformers/js/src/JSTransformer.js | 8 ++++- 12 files changed, 108 insertions(+), 8 deletions(-) create mode 100644 packages/core/integration-tests/test/integration/css-module-self-references/a/index.js create mode 100644 packages/core/integration-tests/test/integration/css-module-self-references/a/package.json create mode 100644 packages/core/integration-tests/test/integration/css-module-self-references/b/index.js create mode 100644 packages/core/integration-tests/test/integration/css-module-self-references/b/package.json create mode 100644 packages/core/integration-tests/test/integration/css-module-self-references/bar.module.css diff --git a/packages/core/core/src/AssetGraph.js b/packages/core/core/src/AssetGraph.js index 81059f8a461..c7fcab88421 100644 --- a/packages/core/core/src/AssetGraph.js +++ b/packages/core/core/src/AssetGraph.js @@ -455,6 +455,10 @@ export default class AssetGraph extends ContentGraph { let dependentAsset = assetsByKey.get(dep.specifier); if (dependentAsset) { dependentAssets.push(dependentAsset); + if (dependentAsset.id === asset.id) { + // Don't orphan circular dependencies. + isDirect = true; + } } } let id = this.addNode(nodeFromAsset(asset)); diff --git a/packages/core/core/src/public/Asset.js b/packages/core/core/src/public/Asset.js index 88372c2f0c2..1c706577889 100644 --- a/packages/core/core/src/public/Asset.js +++ b/packages/core/core/src/public/Asset.js @@ -271,6 +271,19 @@ export class MutableAsset extends BaseAsset implements IMutableAsset { this.#asset.value.sideEffects = sideEffects; } + get uniqueKey(): ?string { + return this.#asset.value.uniqueKey; + } + + set uniqueKey(uniqueKey: ?string): void { + if (this.#asset.value.uniqueKey != null) { + throw new Error( + "Cannot change an asset's uniqueKey after it has been set.", + ); + } + this.#asset.value.uniqueKey = uniqueKey; + } + get symbols(): IMutableAssetSymbols { return new MutableAssetSymbols(this.#asset.options, this.#asset.value); } diff --git a/packages/core/integration-tests/test/css-modules.js b/packages/core/integration-tests/test/css-modules.js index aaac8804e54..9e9b3c738fa 100644 --- a/packages/core/integration-tests/test/css-modules.js +++ b/packages/core/integration-tests/test/css-modules.js @@ -715,4 +715,38 @@ describe('css modules', () => { assert.deepEqual(res[0][0], 'mainJs'); assert(res[0][1].includes('container') && res[0][1].includes('expand')); }); + + it('should allow css modules to be shared between targets', async function () { + let b = await bundle([ + path.join(__dirname, '/integration/css-module-self-references/a'), + path.join(__dirname, '/integration/css-module-self-references/b'), + ]); + + assertBundles(b, [ + { + name: 'main.css', + assets: ['bar.module.css'], + }, + { + name: 'main.css', + assets: ['bar.module.css'], + }, + { + name: 'main.js', + assets: ['index.js', 'bar.module.css'], + }, + { + name: 'main.js', + assets: ['index.js', 'bar.module.css'], + }, + { + name: 'module.js', + assets: ['index.js', 'bar.module.css'], + }, + { + name: 'module.js', + assets: ['index.js', 'bar.module.css'], + }, + ]); + }); }); diff --git a/packages/core/integration-tests/test/integration/css-module-self-references/a/index.js b/packages/core/integration-tests/test/integration/css-module-self-references/a/index.js new file mode 100644 index 00000000000..4f8f5ed1ab1 --- /dev/null +++ b/packages/core/integration-tests/test/integration/css-module-self-references/a/index.js @@ -0,0 +1,3 @@ +import foo from '../bar.module.css'; + +console.log('a', foo); diff --git a/packages/core/integration-tests/test/integration/css-module-self-references/a/package.json b/packages/core/integration-tests/test/integration/css-module-self-references/a/package.json new file mode 100644 index 00000000000..4b54a798902 --- /dev/null +++ b/packages/core/integration-tests/test/integration/css-module-self-references/a/package.json @@ -0,0 +1,5 @@ +{ + "main": "dist/main.js", + "module": "dist/module.js", + "source": "index.js" +} diff --git a/packages/core/integration-tests/test/integration/css-module-self-references/b/index.js b/packages/core/integration-tests/test/integration/css-module-self-references/b/index.js new file mode 100644 index 00000000000..b0bf7d0843f --- /dev/null +++ b/packages/core/integration-tests/test/integration/css-module-self-references/b/index.js @@ -0,0 +1,3 @@ +import foo from '../bar.module.css'; + +console.log('b', foo); diff --git a/packages/core/integration-tests/test/integration/css-module-self-references/b/package.json b/packages/core/integration-tests/test/integration/css-module-self-references/b/package.json new file mode 100644 index 00000000000..4b54a798902 --- /dev/null +++ b/packages/core/integration-tests/test/integration/css-module-self-references/b/package.json @@ -0,0 +1,5 @@ +{ + "main": "dist/main.js", + "module": "dist/module.js", + "source": "index.js" +} diff --git a/packages/core/integration-tests/test/integration/css-module-self-references/bar.module.css b/packages/core/integration-tests/test/integration/css-module-self-references/bar.module.css new file mode 100644 index 00000000000..1a24f8633c1 --- /dev/null +++ b/packages/core/integration-tests/test/integration/css-module-self-references/bar.module.css @@ -0,0 +1,8 @@ +.foo { + composes: composed; + color: white; +} + +.composed { + background: pink; +} \ No newline at end of file diff --git a/packages/core/types/index.js b/packages/core/types/index.js index 50cb98f4a73..99e80f63c1a 100644 --- a/packages/core/types/index.js +++ b/packages/core/types/index.js @@ -749,6 +749,12 @@ export interface MutableAsset extends BaseAsset { * This is initially set by the resolver, but can be overridden by transformers. */ sideEffects: boolean; + /** + * When a transformer returns multiple assets, it can give them unique keys to identify them. + * This can be used to find assets during packaging, or to create dependencies between multiple + * assets returned by a transformer by using the unique key as the dependency specifier. + */ + uniqueKey: ?string; /** The symbols that the asset exports. */ +symbols: MutableAssetSymbols; diff --git a/packages/packagers/css/src/CSSPackager.js b/packages/packagers/css/src/CSSPackager.js index c86fef025b0..0bf0339a8ac 100644 --- a/packages/packagers/css/src/CSSPackager.js +++ b/packages/packagers/css/src/CSSPackager.js @@ -36,6 +36,7 @@ export default (new Packager({ // Hoist unresolved external dependencies (i.e. http: imports) if ( node.value.priority === 'sync' && + !bundleGraph.isDependencySkipped(node.value) && !bundleGraph.getResolvedAsset(node.value, bundle) ) { hoistedImports.push(node.value.specifier); diff --git a/packages/transformers/css/src/CSSTransformer.js b/packages/transformers/css/src/CSSTransformer.js index 0cd95516354..5aeef13ed4b 100644 --- a/packages/transformers/css/src/CSSTransformer.js +++ b/packages/transformers/css/src/CSSTransformer.js @@ -196,6 +196,8 @@ export default (new Transformer({ locals.set(exports[key].name, key); } + asset.uniqueKey ??= asset.id; + let seen = new Set(); let add = key => { if (seen.has(key)) { @@ -209,13 +211,16 @@ export default (new Transformer({ for (let ref of e.composes) { s += ' '; if (ref.type === 'local') { - add(nullthrows(locals.get(ref.name))); - s += - '${' + - `module.exports[${JSON.stringify( - nullthrows(locals.get(ref.name)), - )}]` + - '}'; + let exported = nullthrows(locals.get(ref.name)); + add(exported); + s += '${' + `module.exports[${JSON.stringify(exported)}]` + '}'; + asset.addDependency({ + specifier: nullthrows(asset.uniqueKey), + specifierType: 'esm', + symbols: new Map([ + [exported, {local: ref.name, isWeak: false, loc: null}], + ]), + }); } else if (ref.type === 'global') { s += ref.name; } else if (ref.type === 'dependency') { @@ -242,6 +247,13 @@ export default (new Transformer({ // to the JS so the symbol is retained during tree-shaking. if (e.isReferenced) { s += `module.exports[${JSON.stringify(key)}];\n`; + asset.addDependency({ + specifier: nullthrows(asset.uniqueKey), + specifierType: 'esm', + symbols: new Map([ + [key, {local: exports[key].name, isWeak: false, loc: null}], + ]), + }); } js += s; diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index a28636c2e8b..9b1429ed8b2 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -806,8 +806,14 @@ export default (new Transformer({ }); } + // Use the asset id as a unique key if one has not already been set. + // This lets us create a dependency on the asset itself by using it as a specifier. + // Using the unique key ensures that the dependency always resolves to the correct asset, + // even if it came from a transformer that produced multiple assets (e.g. css modules). + // Also avoids needing a resolution request. + asset.uniqueKey ||= asset.id; asset.addDependency({ - specifier: `./${path.basename(asset.filePath)}`, + specifier: asset.uniqueKey, specifierType: 'esm', symbols, });