Improve maintainability of vendor prefix stripping - #14253
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the runtime regex-based stripping of vendor-prefixed CSS (used to reduce inline AMP/Lite CSS size) with a PostCSS + Autoprefixer-based approach driven by an explicit browserslist configuration for the ws-nextjs-app workspace.
Changes:
- Adds a
browserslisttarget list tows-nextjs-app/package.jsonand uses it to drive prefix removal decisions. - Replaces the regex vendor-prefix stripping in
ws-nextjs-app/pages/_document.page.tsxwithpostcss([autoprefixer({ remove: true })]), with error logging + fallback to unprocessed CSS. - Adds
autoprefixerandpostcssdependencies (and updates lockfile), and introduces a dedicated log code for Autoprefixer failures.
Reviewed changes
Copilot reviewed 3 out of 13 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| yarn.lock | Adds lock entries for autoprefixer, postcss, and their dependency graph. |
| ws-nextjs-app/pages/_document.page.tsx | Switches AMP/Lite inline CSS “optimisation” from regex stripping to Autoprefixer/PostCSS with logging + fallback; applies to both AMP and Lite branches. |
| ws-nextjs-app/package.json | Introduces workspace browserslist config and adds runtime deps needed to run PostCSS/Autoprefixer server-side. |
| src/app/lib/logger.const.js | Adds AMP_LITE_CSS_AUTOPREFIXER_ERROR log code for runtime failures. |
Comments suppressed due to low confidence (1)
ws-nextjs-app/pages/_document.page.tsx:70
- The previous AMP optimisation also performed small, safe minifications (e.g. removing
;}and collapsing repeated whitespace).optimiseCssPrefixesno longer does this, which can inflate the inline AMP/Lite <style> and make it easier to hit AMP’s 75KB CSS limit again.
const optimiseCssPrefixes = (css: string): string => {
try {
return postcss([autoprefixerPlugin]).process(css, { from: undefined }).css;
} catch (e) {
| const autoprefixerPlugin = autoprefixer({ | ||
| overrideBrowserslist: targetBrowsers, | ||
| remove: true, | ||
| }); |
There was a problem hiding this comment.
This change would stop it adding any genuinely needed prefixes that emotion may miss, I'd rather leave this unchanged
- Introduced a new launch configuration for profiling Next.js production builds. - Enhanced logging with AMP/Lite CSS size metrics. - Implemented `pruneUnusedCssCustomProperties` to remove unused CSS custom properties. - Added tests for `pruneUnusedCssCustomProperties` to ensure correct functionality. [copilot]
… config [copilot]
03092be to
cf0b4fd
Compare
andrewscfc
changed the title
| const logger = nodeLogger(__filename); | ||
|
|
||
| const autoprefixerPlugin = autoprefixer({ | ||
| overrideBrowserslist: targetBrowsers, | ||
| remove: true, | ||
| }); | ||
|
|
||
| const optimiseCssPrefixes = (css: string): string => { | ||
| try { | ||
| return postcss([autoprefixerPlugin]).process(css, { from: undefined }).css; | ||
| } catch (e) { | ||
| logger.error(logCodes.AMP_LITE_CSS_AUTOPREFIXER_ERROR, { | ||
| message: e instanceof Error ? e.message : String(e), | ||
| stack: e instanceof Error ? e.stack : undefined, | ||
| }); | ||
| return css; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Copilot informed me that cascade is only needed where multi-line css is passed in, the css is already minified so it will have no effect.
Given this logic runs in a lambda and different pages will include different css I think the benefit of the cache would be limited. I'll look again at this for further optimisation
|
Here's a size comparison before and after this PR, you'll see the size has gone up a bit but it was stripping to much according to the explanation below: AI:Compared AMP inlined CSS size before/after this PR, on
The size increase is expected and is the cost of fixing a real functional regression: the old regex approach blindly stripped every vendor-prefixed declaration, including properties with no safe removal path ( |
There was a problem hiding this comment.
| Page | Avg transform time | P95 transform time |
|---|---|---|
AMP /news/articles/c0g992jmmkko.amp |
16.37 ms | 29.70 ms |
Lite /gahuza/articles/cey23zx8wx8o.lite |
15.85 ms | 32.92 ms |
Did a quick isolation of the autoprefixer visiting local pages using AI, won't be fully accurate to production, but will give us a rough idea for how much processing time this might add
The script it produced and ran:
Code
for i in $(seq 1 40); do curl -fsS "http://127.0.0.1:7081/news/articles/c0g992jmmkko.amp" >/dev/null && break; sleep 2; done && node - <<'NODE'
const { performance } = require('node:perf_hooks');
const autoprefixer = require('autoprefixer');
const postcss = require('postcss');
const pkg = require('./package.json');
const urls = {
amp: 'http://127.0.0.1:7081/news/articles/c0g992jmmkko.amp',
lite: 'http://127.0.0.1:7081/gahuza/articles/cey23zx8wx8o.lite',
};
const extractAmpCss = html => {
const m = html.match(/<style[^>]*amp-custom[^>]*>([\s\S]*?)<\/style>/i);
return m ? m[1] : '';
};
const extractLiteCss = html => {
const matches = [...html.matchAll(/<style[^>]*>([\s\S]*?)<\/style>/gi)];
return matches.map(m => m[1]).join('');
};
const processor = postcss([
autoprefixer({ overrideBrowserslist: pkg.browserslist }),
]);
const optimise = css => processor.process(css, { from: undefined }).css;
const pct = (a,b) => (a === 0 ? 0 : ((b-a)/a)*100);
(async () => {
const results = [];
for (const [kind, url] of Object.entries(urls)) {
const res = await fetch(url);
if (!res.ok) throw new Error(`${kind} fetch failed: ${res.status}`);
const html = await res.text();
const rawCss = kind === 'amp' ? extractAmpCss(html) : extractLiteCss(html);
if (!rawCss) throw new Error(`${kind} css extraction failed`);
const beforeBytes = Buffer.byteLength(rawCss, 'utf8');
const afterCss = optimise(rawCss);
const afterBytes = Buffer.byteLength(afterCss, 'utf8');
const iterations = 50;
const timings = [];
for (let i = 0; i < iterations; i += 1) {
const t0 = performance.now();
optimise(rawCss);
timings.push(performance.now() - t0);
}
timings.sort((a,b) => a-b);
const avgMs = timings.reduce((a,b)=>a+b,0) / iterations;
const p95Ms = timings[Math.floor(iterations * 0.95) - 1];
results.push({
kind,
beforeBytes,
afterBytes,
bytesDelta: afterBytes - beforeBytes,
pctDelta: pct(beforeBytes, afterBytes),
avgMs,
p95Ms,
});
}
const totalBefore = results.reduce((s, r) => s + r.beforeBytes, 0);
const totalAfter = results.reduce((s, r) => s + r.afterBytes, 0);
console.log(JSON.stringify({
results,
total: {
beforeBytes: totalBefore,
afterBytes: totalAfter,
bytesDelta: totalAfter - totalBefore,
pctDelta: pct(totalBefore, totalAfter),
},
}, null, 2));
})();
This looks good compared to the 100ms-ish the old solution seemed to add to response times |
|
Checked the steps mentioned on the PR under testing and they all look good. All tests pass. Conflicts needs resolving. |
PR Description generated by AI, verified by me
Summary
Replaces the regex-based vendor-prefix stripping in AMP/Lite CSS (introduced in #14229 to fix the AMP 75KB CSS limit) with a safer, standards-driven approach using Autoprefixer and a proper
browserslistconfig.The previous approach used a hand-written regex to blindly strip any
-webkit-/-moz-/-ms-/-o-prefixed CSS. This risked breaking properties Autoprefixer doesn't manage at all — e.g.-webkit-overflow-scrolling(no standard alternative exists for iOS momentum scrolling), and-webkit-line-clamp/-webkit-box-orient(multi-line text truncation — a standardline-clampproperty now exists in the CSS Overflow Module Level 4 spec, but isn't yet supported by any browser in our target list) — since a regex can't distinguish "safe duplicate" from "the only way to get this behaviour in the browsers we support."Autoprefixer solves this correctly: it uses real caniuse compatibility data against our declared target browsers to decide what's safe to remove, and leaves untouched anything it doesn't recognise as a trackable prefix. This required first adding a proper
browserslistconfig to the project, which didn't previously exist (Next.js was falling back to a very broad legacy default, including IE11 support the project doesn't actually need).Code changes
browserslistconfig to package.json, based on the project's legacy babel.config.main.js target list, with:ie >= 11dropped (IE11 has no support for CSS custom properties, which our CSS Modules theming relies on entirely — so it was already incompatible regardless of this config)op_mini >= 18corrected toop_mini all(Opera Mini's data-saving proxy isn't versioned in caniuse-lite the way other browsers are)op_mob >= 80added (previously missing entirely; targets the modern Chromium-based Opera Mobile only, excluding pre-80 Presto-engine versions which Browserslist itself marks asdead)firefox/and_ffraised to>= 52andsafari/ios_safraised to>= 12.1/>= 12.2— these are the actual minimum versions in the legacy target list once the incorrect entries above are fixed, and keeping them precise avoids Autoprefixer being overly conservativeautoprefixerandpostcssas direct dependencies of the ws-nextjs-app workspacestripVendorPrefixes/optimiseAmpCss(regex-based) in _document.page.tsx withoptimiseCssPrefixes, extracted into its own module (optimiseCssPrefixes), which runs the combined CSS throughpostcss([autoprefixer({ overrideBrowserslist, remove: true })])AMP_LITE_CSS_AUTOPREFIXER_ERRORlog code — if Autoprefixer throws for any reason, the error is logged and the unprocessed CSS is used as a fallback rather than breaking the renderoptimiseCssPrefixescovering: removing unnecessary legacy prefixes, preserving prefixes Autoprefixer doesn't track (that this codebase depends on), adding a genuinely-required prefix, no-op/empty-input behaviour, and the PostCSS-failure fallback/logging pathTesting
yarn workspace simorgh-nextjs test utilities/optimiseCssPrefixes— unit tests for the new moduleyarn test:integration(from the repo root) — builds, starts the server, and runs all Canonical/AMP/Lite integration suites end-to-end automatically; confirms vendor prefixes are stripped from real rendered output, while genuinely-needed prefixes are preserved.yarn workspace simorgh-nextjs build:local && yarn workspace simorgh-nextjs start(in one terminal), thenyarn workspace simorgh-nextjs test:integration:amp/test:integration:lite(in another) — these scoped scripts assume a server is already running on port 7081 and won't build/start one for you.npx browserslistfrom ws-nextjs-app to confirm the config resolves without warnings