diff --git a/package-lock.json b/package-lock.json index d1eab1489..9bddfa443 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27023,7 +27023,8 @@ "clean-css": "5.3.3", "node-fetch": "2.7.0", "path-to-regexp": "0.1.10", - "playwright-chromium": "1.25.0" + "playwright-chromium": "1.25.0", + "postcss": "8.4.31" } }, "packages/sui-critical-css-middleware": { diff --git a/package.json b/package.json index e677b7c21..3d6ff4ed0 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "test:server:ci": "npm run test:server", "test:client": "npx @s-ui/test browser -P 'packages/**/!(server)/*Spec.js' -H", "test:client:watch": "npm run test:client -- --watch", - "test:server": "npx @s-ui/test server -P 'packages/{sui-test-contract,sui-js-compiler}/**/server/*Spec.js'", + "test:server": "npx @s-ui/test server -P 'packages/{sui-test-contract,sui-js-compiler,sui-critical-css}/**/server/*Spec.js'", "test:server:watch": "npm run test:server -- --watch", "test:e2e": "node ./packages/sui-studio/test/server/integration/static-server.js ./packages/sui-studio/test/server/integration/sample-studio/public && npx @s-ui/test-e2e --baseUrl=http://localhost:1234", "pre-commit": "sui-lint js --staged && sui-lint sass --staged", diff --git a/packages/sui-critical-css/package.json b/packages/sui-critical-css/package.json index dd27e8010..9409384e8 100644 --- a/packages/sui-critical-css/package.json +++ b/packages/sui-critical-css/package.json @@ -5,13 +5,14 @@ "type": "module", "main": "src/index.js", "scripts": { - "test": "echo \"Error: no test specified\" && exit 1" + "test": "sui-test server" }, "dependencies": { "clean-css": "5.3.3", "node-fetch": "2.7.0", "path-to-regexp": "0.1.10", "playwright-chromium": "1.25.0", + "postcss": "8.4.31", "@s-ui/critical-css-middleware": "1" }, "keywords": [], diff --git a/packages/sui-critical-css/src/covered-css.js b/packages/sui-critical-css/src/covered-css.js new file mode 100644 index 000000000..da85a073f --- /dev/null +++ b/packages/sui-critical-css/src/covered-css.js @@ -0,0 +1,114 @@ +import postcss from 'postcss' + +// Chrome's CSS coverage reports ranges that cover the style rule ONLY. The `@media`, +// `@layer`, `@supports` or `@container` block the rule lives in is never part of the +// range, so slicing the raw bytes out of `entry.text` and concatenating them produces +// CSS that has lost every at-rule wrapper. Two things break as a result: +// +// 1. Nothing is layered any more, so ties that the layer order used to decide now +// fall back to source order. A `@layer base` reset extracted after a +// `@layer utilities` rule of the same specificity silently starts winning. +// 2. Media queries are flattened, so rules only meant for the extraction viewport +// apply at every width. +// +// So instead of slicing bytes, parse the stylesheet and rebuild it: keep every +// declaration-holding node whose source range intersects a covered range, together +// with its whole chain of ancestor at-rules. +const KEEP_AT_RULE = 'layer' + +const holdsDeclarations = node => + node.type === 'rule' || (node.type === 'atrule' && node.nodes?.some(child => child.type === 'decl')) + +// `@layer a, b, c;` is a statement, not a style rule, so coverage never marks it as +// used — yet it is the only thing that fixes the order of the layers. Always keep it. +const isLayerStatement = node => + node.type === 'atrule' && node.nodes === undefined && node.name.toLowerCase() === KEEP_AT_RULE + +// One statement per layer name, because clean-css 5.3.3 mis-parses a bodyless `@layer` +// that names more than one layer: it drops the statement AND every declaration of the +// rule that follows it (`@layer a,b;.x{color:red}` minifies to nothing at all). A +// sequence of single-name statements establishes exactly the same order. +const splitLayerStatement = node => node.params.split(',').map(name => `@${node.name} ${name.trim()};`) + +const intersectsCoveredRange = (node, ranges) => { + const start = node.source?.start?.offset + const end = node.source?.end?.offset + + if (start === undefined || end === undefined) return false + + return ranges.some(range => range.start < end && range.end > start) +} + +/** + * Rebuild the used part of a stylesheet from its Chrome CSS coverage ranges, keeping + * the at-rule context of every rule it keeps. + * + * @param {object} params + * @param {string} params.text Full source of the stylesheet, as reported by coverage. + * @param {Array<{start: number, end: number}>} params.ranges Covered byte ranges. + * @returns {string} CSS containing only the covered rules, still wrapped in their + * original at-rules, with every `@layer` statement hoisted to the top. + */ +export const rebuildCoveredCSS = ({text, ranges}) => { + const root = postcss.parse(text) + const keep = new Set() + const layerStatements = [] + const nestedLayerStatements = [] + + const keepWithAncestors = node => { + for (let current = node; current && current.type !== 'root'; current = current.parent) keep.add(current) + } + + root.walk(node => { + if (isLayerStatement(node)) { + if (node.parent.type === 'root') return layerStatements.push(...splitLayerStatement(node)) + + // A statement inside `@supports` or `@media` only registers its layers while that + // condition holds, and one inside another `@layer` names sublayers of it, so it + // cannot be hoisted out of its parent the way a top-level one can. Keep it in place. + return nestedLayerStatements.push(node) + } + if (holdsDeclarations(node) && intersectsCoveredRange(node, ranges)) keepWithAncestors(node) + }) + + // Replacing them during the walk above would make postcss visit the statements the split + // produces, so they are only rewritten once the walk is over. + nestedLayerStatements.forEach(node => { + const replacements = splitLayerStatement(node).map(statement => postcss.parse(statement).first) + + node.replaceWith(...replacements) + replacements.forEach(keepWithAncestors) + }) + + const discarded = [] + root.walk(node => { + if ((node.type === 'rule' || node.type === 'atrule') && keep.has(node) === false) discarded.push(node) + }) + discarded.forEach(node => node.remove()) + + // Coverage entries are not guaranteed to be in document order, so a `@layer` + // statement could otherwise end up after a rule that already registered the layers + // in the wrong order. Hoisting makes the result independent of entry order. + return [...layerStatements, root.toString()].filter(Boolean).join('\n') +} + +/** + * Same as `rebuildCoveredCSS`, but never throws: a stylesheet postcss cannot parse + * falls back to the old byte-slicing behaviour instead of failing the whole + * extraction. The fallback loses at-rule context, so it is logged loudly. + * + * @param {object} entry A single Chrome CSS coverage entry. + * @returns {string} + */ +export const rebuildCoveredCSSFromEntry = entry => { + try { + return rebuildCoveredCSS(entry) + } catch (error) { + console.warn( + `[ko] Could not parse ${entry.url ?? 'a stylesheet'} (${error.message}).`, + 'Falling back to raw coverage ranges, so its @media and @layer context is lost.' + ) + + return entry.ranges.reduce((css, range) => css + entry.text.slice(range.start, range.end), '') + } +} diff --git a/packages/sui-critical-css/src/extract-from-url.js b/packages/sui-critical-css/src/extract-from-url.js index 30c7c3e48..a348d5685 100644 --- a/packages/sui-critical-css/src/extract-from-url.js +++ b/packages/sui-critical-css/src/extract-from-url.js @@ -2,6 +2,7 @@ import CleanCSS from 'clean-css' import {chromium} from 'playwright-chromium' import {blockedResourceTypes, skippedResources} from './config.js' +import {rebuildCoveredCSSFromEntry} from './covered-css.js' let browser @@ -84,9 +85,7 @@ export async function extractCSSFromUrl({customHeaders, height, url, userAgent, let coveredCSS = '' for (const entry of coverage) { - for (const range of entry.ranges) { - coveredCSS += entry.text.slice(range.start, range.end) - } + coveredCSS += rebuildCoveredCSSFromEntry(entry) } // Close the browser to close the connection and free up resources diff --git a/packages/sui-critical-css/test/server/covered-cssSpec.js b/packages/sui-critical-css/test/server/covered-cssSpec.js new file mode 100644 index 000000000..56a58943e --- /dev/null +++ b/packages/sui-critical-css/test/server/covered-cssSpec.js @@ -0,0 +1,137 @@ +import {expect} from 'chai' +import CleanCSS from 'clean-css' + +import {rebuildCoveredCSS, rebuildCoveredCSSFromEntry} from '../../src/covered-css.js' + +// Chrome reports coverage as byte ranges into the stylesheet source, and the range of a +// used rule covers that rule ONLY — never the at-rule it is nested in. These helpers +// build such a range from a substring so the specs read like the real input. +const rangeOf = (text, snippet) => { + const start = text.indexOf(snippet) + + if (start === -1) throw new Error(`Fixture does not contain "${snippet}"`) + + return {start, end: start + snippet.length} +} + +const rebuild = (text, ...snippets) => + rebuildCoveredCSS({text, ranges: snippets.map(snippet => rangeOf(text, snippet))}) + +describe('@s-ui/critical-css covered-css', () => { + it('keeps the @media a covered rule lives in', () => { + const text = '@media (min-width:600px){.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('keeps the @layer a covered rule lives in', () => { + const text = '@layer utilities{.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('keeps the whole chain of nested at-rules', () => { + const text = '@supports (display:grid){@media (min-width:600px){@layer base{.used{color:red}}}}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('drops uncovered siblings but keeps the at-rule of the covered one', () => { + const text = '@media print{.used{color:red}.unused{color:blue}}' + + expect(rebuild(text, '.used{color:red}')).to.equal('@media print{.used{color:red}}') + }) + + it('drops an at-rule with no covered rule at all', () => { + const text = '.used{color:red}@media print{.unused{color:blue}}' + + expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}') + }) + + it('keeps unlayered covered rules as they are', () => { + const text = '.used{color:red}.unused{color:blue}' + + expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}') + }) + + it('keeps a @layer statement even though coverage never marks it as used', () => { + const text = '@layer base,utilities;@layer utilities{.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal( + '@layer base;\n@layer utilities;\n@layer utilities{.used{color:red}}' + ) + }) + + it('keeps a @layer statement even when nothing else is covered', () => { + expect(rebuildCoveredCSS({text: '@layer base,utilities;.unused{color:blue}', ranges: []})).to.equal( + '@layer base;\n@layer utilities;' + ) + }) + + it('hoists @layer statements above the rules, whatever their original position', () => { + const text = '@layer utilities{.used{color:red}}@layer base,utilities;' + + expect(rebuild(text, '.used{color:red}')).to.equal( + '@layer base;\n@layer utilities;\n@layer utilities{.used{color:red}}' + ) + }) + + it('does not hoist a @layer statement out of the at-rule that guards it', () => { + const text = '@supports (color:lab(0 0 0)){@layer a,b;}@layer a{.used{color:red}}' + + // Hoisting it would register the order even where the condition does not hold. + expect(rebuild(text, '.used{color:red}')).to.equal( + '@supports (color:lab(0 0 0)){@layer a;@layer b;}@layer a{.used{color:red}}' + ) + }) + + it('does not hoist a @layer statement out of the layer it names sublayers of', () => { + const text = '@layer a{@layer b,c;}@layer a{.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal('@layer a{@layer b;@layer c;}@layer a{.used{color:red}}') + }) + + it('splits a multi-name @layer statement so clean-css does not swallow the next rule', () => { + const text = '@layer base,utilities;:root{--brand:red}' + const rebuilt = rebuildCoveredCSS({text, ranges: [rangeOf(text, ':root{--brand:red}')]}) + + // `@layer base,utilities;:root{--brand:red}` minifies to `:root{}` on clean-css + // 5.3.3: the statement is dropped and the rule after it loses every declaration. + expect(new CleanCSS({level: 2}).minify(rebuilt).styles).to.contain('--brand:red') + }) + + it('keeps a covered at-rule that holds declarations directly, such as @font-face', () => { + const text = '@font-face{font-family:F;src:url(f.woff2)}.unused{color:blue}' + + expect(rebuild(text, '@font-face{font-family:F;src:url(f.woff2)}')).to.equal( + '@font-face{font-family:F;src:url(f.woff2)}' + ) + }) + + it('returns an empty string when nothing is covered', () => { + expect(rebuildCoveredCSS({text: '@media print{.unused{color:blue}}', ranges: []})).to.equal('') + }) + + it('does not lose a rule whose range only partially overlaps it', () => { + const text = '@layer base{.used{color:red}}' + const rule = rangeOf(text, '.used{color:red}') + + expect(rebuildCoveredCSS({text, ranges: [{start: rule.start, end: rule.start + 3}]})).to.equal(text) + }) + + describe('rebuildCoveredCSSFromEntry', () => { + it('falls back to the raw coverage ranges when the stylesheet cannot be parsed', () => { + const text = '.used{color:red}@media{' + const entry = {url: 'https://example.com/broken.css', text, ranges: [rangeOf(text, '.used{color:red}')]} + + expect(rebuildCoveredCSSFromEntry(entry)).to.equal('.used{color:red}') + }) + + it('rebuilds a parseable stylesheet with its at-rule context', () => { + const text = '@layer utilities{.used{color:red}}' + const entry = {url: 'https://example.com/ok.css', text, ranges: [rangeOf(text, '.used{color:red}')]} + + expect(rebuildCoveredCSSFromEntry(entry)).to.equal(text) + }) + }) +})