Skip to content

Commit

Permalink
Fix CSS tree shaking with 'build --no-scope-hoist' (#5728)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mischnic
mischnic committed Oct 8, 2021
1 parent ea0f4e4 commit 415710f
Show file tree
Hide file tree
Showing 9 changed files with 128 additions and 55 deletions.
7 changes: 6 additions & 1 deletion packages/core/core/src/AssetGraph.js
View file
Expand Up @@ -34,6 +34,7 @@ type InitOpts = {|
type SerializedAssetGraph = {|
...SerializedContentGraph<AssetGraphNode>,
hash?: ?string,
symbolPropagationRan: boolean,
|};

export function nodeFromDep(dep: Dependency): DependencyNode {
Expand Down Expand Up @@ -101,12 +102,14 @@ export default class AssetGraph extends ContentGraph<AssetGraphNode> {
onNodeRemoved: ?(nodeId: NodeId) => mixed;
hash: ?string;
envCache: Map<string, Environment>;
symbolPropagationRan: boolean;

constructor(opts: ?SerializedAssetGraph) {
if (opts) {
let {hash, ...rest} = opts;
let {hash, symbolPropagationRan, ...rest} = opts;
super(rest);
this.hash = hash;
this.symbolPropagationRan = symbolPropagationRan;
} else {
super();
this.setRootNodeId(
Expand All @@ -118,6 +121,7 @@ export default class AssetGraph extends ContentGraph<AssetGraphNode> {
);
}
this.envCache = new Map();
this.symbolPropagationRan = false;
}

// $FlowFixMe[prop-missing]
Expand All @@ -130,6 +134,7 @@ export default class AssetGraph extends ContentGraph<AssetGraphNode> {
return {
...super.serialize(),
hash: this.hash,
symbolPropagationRan: this.symbolPropagationRan,
};
}

Expand Down
20 changes: 16 additions & 4 deletions packages/core/core/src/BundleGraph.js
View file
Expand Up @@ -79,6 +79,7 @@ type SerializedBundleGraph = {|
bundleContentHashes: Map<string, string>,
assetPublicIds: Set<string>,
publicIdByAssetId: Map<string, string>,
symbolPropagationRan: boolean,
|};

function makeReadOnlySet<T>(set: Set<T>): $ReadOnlySet<T> {
Expand All @@ -105,22 +106,26 @@ export default class BundleGraph {
_bundleContentHashes: Map<string, string>;
_targetEntryRoots: Map<ProjectPath, FilePath> = new Map();
_graph: ContentGraph<BundleGraphNode, BundleGraphEdgeType>;
_symbolPropagationRan /*: boolean*/;

constructor({
graph,
publicIdByAssetId,
assetPublicIds,
bundleContentHashes,
symbolPropagationRan,
}: {|
graph: ContentGraph<BundleGraphNode, BundleGraphEdgeType>,
publicIdByAssetId: Map<string, string>,
assetPublicIds: Set<string>,
bundleContentHashes: Map<string, string>,
symbolPropagationRan: boolean,
|}) {
this._graph = graph;
this._assetPublicIds = assetPublicIds;
this._publicIdByAssetId = publicIdByAssetId;
this._bundleContentHashes = bundleContentHashes;
this._symbolPropagationRan = symbolPropagationRan;
}

static fromAssetGraph(
Expand Down Expand Up @@ -203,6 +208,7 @@ export default class BundleGraph {
assetPublicIds,
bundleContentHashes: new Map(),
publicIdByAssetId,
symbolPropagationRan: assetGraph.symbolPropagationRan,
});
}

Expand All @@ -213,6 +219,7 @@ export default class BundleGraph {
assetPublicIds: this._assetPublicIds,
bundleContentHashes: this._bundleContentHashes,
publicIdByAssetId: this._publicIdByAssetId,
symbolPropagationRan: this._symbolPropagationRan,
};
}

Expand All @@ -222,6 +229,7 @@ export default class BundleGraph {
assetPublicIds: serialized.assetPublicIds,
bundleContentHashes: serialized.bundleContentHashes,
publicIdByAssetId: serialized.publicIdByAssetId,
symbolPropagationRan: serialized.symbolPropagationRan,
});
}

Expand Down Expand Up @@ -1602,16 +1610,20 @@ export default class BundleGraph {
}
}

getUsedSymbolsAsset(asset: Asset): $ReadOnlySet<Symbol> {
getUsedSymbolsAsset(asset: Asset): ?$ReadOnlySet<Symbol> {
let node = this._graph.getNodeByContentKey(asset.id);
invariant(node && node.type === 'asset');
return makeReadOnlySet(node.usedSymbols);
return this._symbolPropagationRan
? makeReadOnlySet(node.usedSymbols)
: null;
}

getUsedSymbolsDependency(dep: Dependency): $ReadOnlySet<Symbol> {
getUsedSymbolsDependency(dep: Dependency): ?$ReadOnlySet<Symbol> {
let node = this._graph.getNodeByContentKey(dep.id);
invariant(node && node.type === 'dependency');
return makeReadOnlySet(node.usedSymbolsUp);
return this._symbolPropagationRan
? makeReadOnlySet(node.usedSymbolsUp)
: null;
}

merge(other: BundleGraph) {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/core/src/public/BundleGraph.js
View file
Expand Up @@ -294,7 +294,7 @@ export default class BundleGraph<TBundle: IBundle>
.map(bundle => this.#createBundle(bundle, this.#graph, this.#options));
}

getUsedSymbols(v: IAsset | IDependency): $ReadOnlySet<Symbol> {
getUsedSymbols(v: IAsset | IDependency): ?$ReadOnlySet<Symbol> {
if (v instanceof Asset) {
return this.#graph.getUsedSymbolsAsset(assetToAssetValue(v));
} else {
Expand Down
6 changes: 5 additions & 1 deletion packages/core/core/src/requests/AssetGraphRequest.js
View file
Expand Up @@ -203,7 +203,11 @@ export class AssetGraphBuilder {
return dep;
}),
);
if (entryDependencies.some(d => d.value.env.shouldScopeHoist)) {

this.assetGraph.symbolPropagationRan = entryDependencies.some(
d => d.value.env.shouldScopeHoist,
);
if (this.assetGraph.symbolPropagationRan) {
try {
this.propagateSymbols();
} catch (e) {
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/test/PublicBundle.test.js
View file
Expand Up @@ -42,6 +42,7 @@ describe('Public Bundle', () => {
assetPublicIds: new Set(),
publicIdByAssetId: new Map(),
bundleContentHashes: new Map(),
symbolPropagationRan: false,
});
});

Expand Down
41 changes: 41 additions & 0 deletions packages/core/integration-tests/test/postcss.js
View file
Expand Up @@ -153,6 +153,47 @@ describe('postcss', () => {
);
});

it('should produce correct css without symbol propagation for css modules classes with a namespace import', async () => {
let b = await bundle(
path.join(
__dirname,
'/integration/postcss-modules-import-namespace/index.js',
),
{
mode: 'production',
defaultTargetOptions: {
shouldScopeHoist: false,
},
},
);

assertBundles(b, [
{
name: 'index.js',
assets: ['index.js', 'style.module.css'],
},
{
name: 'index.css',
assets: ['global.css', 'style.module.css'],
},
]);

let {output} = await run(b, null, {require: false});
assert(/_b-2_[0-9a-z]/.test(output));

let css = await outputFS.readFile(
b.getBundles().find(b => b.type === 'css').filePath,
'utf8',
);
let includedRules = new Set();
postcss.parse(css).walkRules(rule => {
includedRules.add(rule.selector);
});
assert(includedRules.has('body'));
assert(includedRules.has(`.${output}`));
assert(includedRules.has('.page'));
});

it('should support importing css modules with a non-static namespace import', async () => {
let b = await bundle(
path.join(
Expand Down
8 changes: 6 additions & 2 deletions packages/core/types/index.js
View file
Expand Up @@ -1410,8 +1410,12 @@ export interface BundleGraph<TBundle: Bundle> {
asset: Asset,
boundary: ?Bundle,
): Array<ExportSymbolResolution>;
/** Returns a list of symbols from an asset or dependency that are referenced by a dependent asset. */
getUsedSymbols(Asset | Dependency): $ReadOnlySet<Symbol>;
/**
* Returns a list of symbols from an asset or dependency that are referenced by a dependent asset.
*
* Returns null if symbol propagation didn't run (so the result is unknown).
*/
getUsedSymbols(Asset | Dependency): ?$ReadOnlySet<Symbol>;
/** Returns the common root directory for the entry assets of a target. */
getEntryRoot(target: Target): FilePath;
}
Expand Down
83 changes: 43 additions & 40 deletions packages/packagers/css/src/CSSPackager.js
View file
Expand Up @@ -153,49 +153,52 @@ async function processCSSModule(
let ast: Root = postcss.fromJSON(nullthrows((await asset.getAST())?.program));

let usedSymbols = bundleGraph.getUsedSymbols(asset);
let localSymbols = new Set(
[...asset.symbols].map(([, {local}]) => `.${local}`),
);

let defaultImport = null;
if (usedSymbols.has('default')) {
let incoming = bundleGraph.getIncomingDependencies(asset);
defaultImport = incoming.find(d => d.symbols.hasExportSymbol('default'));
if (defaultImport) {
let loc = defaultImport.symbols.get('default')?.loc;
logger.warn({
message:
'CSS modules cannot be tree shaken when imported with a default specifier',
...(loc && {
codeFrames: [
{
filePath: nullthrows(loc?.filePath ?? defaultImport.sourcePath),
codeHighlights: [{start: loc.start, end: loc.end}],
},
if (usedSymbols != null) {
let localSymbols = new Set(
[...asset.symbols].map(([, {local}]) => `.${local}`),
);

let defaultImport = null;
if (usedSymbols.has('default')) {
let incoming = bundleGraph.getIncomingDependencies(asset);
defaultImport = incoming.find(d => d.symbols.hasExportSymbol('default'));
if (defaultImport) {
let loc = defaultImport.symbols.get('default')?.loc;
logger.warn({
message:
'CSS modules cannot be tree shaken when imported with a default specifier',
...(loc && {
codeFrames: [
{
filePath: nullthrows(loc?.filePath ?? defaultImport.sourcePath),
codeHighlights: [{start: loc.start, end: loc.end}],
},
],
}),
hints: [
`Instead do: import * as style from "${defaultImport.specifier}";`,
],
}),
hints: [
`Instead do: import * as style from "${defaultImport.specifier}";`,
],
documentationURL: 'https://parceljs.org/languages/css/#tree-shaking',
});
documentationURL: 'https://parceljs.org/languages/css/#tree-shaking',
});
}
}
}

if (!defaultImport && !usedSymbols.has('*')) {
let usedLocalSymbols = new Set(
[...usedSymbols].map(
exportSymbol => `.${nullthrows(asset.symbols.get(exportSymbol)).local}`,
),
);
ast.walkRules(rule => {
if (
localSymbols.has(rule.selector) &&
!usedLocalSymbols.has(rule.selector)
) {
rule.remove();
}
});
if (!defaultImport && !usedSymbols.has('*')) {
let usedLocalSymbols = new Set(
[...usedSymbols].map(
exportSymbol =>
`.${nullthrows(asset.symbols.get(exportSymbol)).local}`,
),
);
ast.walkRules(rule => {
if (
localSymbols.has(rule.selector) &&
!usedLocalSymbols.has(rule.selector)
) {
rule.remove();
}
});
}
}

let {content, map} = await postcss().process(ast, {
Expand Down
15 changes: 9 additions & 6 deletions packages/packagers/js/src/ScopeHoistingPackager.js
View file
Expand Up @@ -841,7 +841,7 @@ ${code}
let append = '';

let shouldWrap = this.wrappedAssets.has(asset.id);
let usedSymbols = this.bundleGraph.getUsedSymbols(asset);
let usedSymbols = nullthrows(this.bundleGraph.getUsedSymbols(asset));
let assetId = asset.meta.id;
invariant(typeof assetId === 'string');

Expand All @@ -866,7 +866,8 @@ ${code}
.getIncomingDependencies(asset)
.some(
dep =>
!dep.isEntry && this.bundleGraph.getUsedSymbols(dep).has('*'),
!dep.isEntry &&
nullthrows(this.bundleGraph.getUsedSymbols(dep)).has('*'),
))) ||
// If a symbol is imported (used) from a CJS asset but isn't listed in the symbols,
// we fallback on the namespace object.
Expand Down Expand Up @@ -923,7 +924,7 @@ ${code}
}

let unused = incomingDeps.every(d => {
let symbols = this.bundleGraph.getUsedSymbols(d);
let symbols = nullthrows(this.bundleGraph.getUsedSymbols(d));
return !symbols.has(symbol) && !symbols.has('*');
});
return !unused;
Expand Down Expand Up @@ -977,13 +978,15 @@ ${code}
if (
isWrapped ||
resolved.meta.staticExports === false ||
this.bundleGraph.getUsedSymbols(resolved).has('*')
nullthrows(this.bundleGraph.getUsedSymbols(resolved)).has('*')
) {
let obj = this.getSymbolResolution(asset, resolved, '*', dep);
append += `$parcel$exportWildcard($${assetId}$exports, ${obj});\n`;
this.usedHelpers.add('$parcel$exportWildcard');
} else {
for (let symbol of this.bundleGraph.getUsedSymbols(dep)) {
for (let symbol of nullthrows(
this.bundleGraph.getUsedSymbols(dep),
)) {
if (
symbol === 'default' || // `export * as ...` does not include the default export
symbol === '__esModule'
Expand Down Expand Up @@ -1130,7 +1133,7 @@ ${code}

return (
asset.sideEffects === false &&
this.bundleGraph.getUsedSymbols(asset).size == 0 &&
nullthrows(this.bundleGraph.getUsedSymbols(asset)).size == 0 &&
!this.bundleGraph.isAssetReferenced(this.bundle, asset)
);
}
Expand Down

0 comments on commit 415710f

Please sign in to comment.