-
Notifications
You must be signed in to change notification settings - Fork 280
Improve maintainability of vendor prefix stripping #14253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ca14222
b58824a
4869a1f
28732cd
44ef1d7
225381f
7b207af
cf0b4fd
78a702a
910c04d
1c25fbc
550662f
efdeefd
8cd39c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import loggerMock from '#testHelpers/loggerMock'; | ||
| import optimiseCssPrefixes from '.'; | ||
|
|
||
| describe('optimiseCssPrefixes', () => { | ||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('removes vendor prefixes not needed for the declared target browsers', () => { | ||
| // Old 2009/2012-draft flexbox spec syntax — not needed for any browser in | ||
| // this project's browserslist config. | ||
| const css = | ||
| '.foo{display:-webkit-box;-webkit-box-align:center;align-items:center;}'; | ||
|
|
||
| const result = optimiseCssPrefixes(css); | ||
|
|
||
| expect(result).not.toContain('-webkit-box-align'); | ||
| expect(result).toContain('align-items:center'); | ||
| }); | ||
|
|
||
| it('keeps vendor-prefixed properties that Autoprefixer does not track', () => { | ||
| // None of these appear in Autoprefixer's data set, so they're never | ||
| // touched regardless of the target browsers. They're real properties | ||
| // this codebase relies on (momentum scrolling, text truncation), so | ||
| // this guards against a future dependency bump changing that. | ||
| const css = | ||
| '.foo{-webkit-overflow-scrolling:touch;display:-webkit-box;-webkit-line-clamp:4;-webkit-box-orient:vertical;overflow:hidden;}'; | ||
|
|
||
| const result = optimiseCssPrefixes(css); | ||
|
|
||
| expect(result).toContain('-webkit-overflow-scrolling:touch'); | ||
| expect(result).toContain('-webkit-line-clamp:4'); | ||
| expect(result).toContain('-webkit-box-orient:vertical'); | ||
| }); | ||
|
|
||
| it('adds a vendor prefix that is genuinely required for a declared target browser', () => { | ||
| // and_chr/chrome need -webkit-clip-path in some of the versions this | ||
| // project declares support for. | ||
| const css = '.foo{clip-path:circle(50%);}'; | ||
|
|
||
| const result = optimiseCssPrefixes(css); | ||
|
|
||
| expect(result).toContain('-webkit-clip-path:circle(50%)'); | ||
| expect(result).toContain('clip-path:circle(50%)'); | ||
| }); | ||
|
|
||
| it('returns the CSS unchanged when there is nothing to optimise', () => { | ||
| const css = '.foo{color:red;}'; | ||
|
|
||
| expect(optimiseCssPrefixes(css)).toBe(css); | ||
| }); | ||
|
|
||
| it('returns an empty string for empty input', () => { | ||
| expect(optimiseCssPrefixes('')).toBe(''); | ||
| }); | ||
|
|
||
| it('logs an error and falls back to the original CSS when PostCSS fails to parse it', () => { | ||
| // Missing closing brace — genuinely invalid CSS that PostCSS's parser | ||
| // will throw a CssSyntaxError on. | ||
| const invalidCss = '.foo{color:red'; | ||
|
|
||
| const result = optimiseCssPrefixes(invalidCss); | ||
|
|
||
| expect(result).toBe(invalidCss); | ||
| expect(loggerMock.error).toHaveBeenCalledWith( | ||
| 'amp_lite_css_autoprefixer_error', | ||
| expect.objectContaining({ message: expect.any(String) }), | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import postcss from 'postcss'; | ||
| import autoprefixer from 'autoprefixer'; | ||
| import nodeLogger from '#lib/logger.node'; | ||
| import logCodes from '#app/lib/logger.const'; | ||
| import { browserslist as targetBrowsers } from '../../package.json'; | ||
|
|
||
| /** | ||
| * Public API for this module: optimiseCssPrefixes (default export) | ||
| * | ||
| * Removes vendor-prefixed CSS that isn't needed for the project's real target | ||
| * browsers (see `browserslist` in package.json), using Autoprefixer's own | ||
| * caniuse-backed compatibility data rather than a hand-rolled pattern match. | ||
| * This correctly keeps properties with no standard equivalent (e.g. | ||
| * `-webkit-overflow-scrolling`) regardless of target, since Autoprefixer only | ||
| * removes prefixes it knows are safe to remove for the given browser list. | ||
| */ | ||
|
|
||
| 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; | ||
| } | ||
| }; | ||
|
Comment on lines
+18
to
+35
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| export default optimiseCssPrefixes; | ||
Uh oh!
There was an error while loading. Please reload this page.