Skip to content

[CLOV-1637] Phase 1: Drop legacy fallback declaration in bpk-themeable-property and delete duplicate dark style#4630

Open
IrinaWei wants to merge 15 commits into
mainfrom
phase2-themeable-property-no-fallback-decl
Open

[CLOV-1637] Phase 1: Drop legacy fallback declaration in bpk-themeable-property and delete duplicate dark style#4630
IrinaWei wants to merge 15 commits into
mainfrom
phase2-themeable-property-no-fallback-decl

Conversation

@IrinaWei
Copy link
Copy Markdown
Contributor

@IrinaWei IrinaWei commented May 25, 2026

Summary

Stacked on top of #4492. Targets the bundle-size pressure surfaced by the large :root { ... } token block introduced in that PR — base.css had grown to ~15.5 KB, pushing Backpack's CSS close to the web-platform limit when consumed in apps like web-platform.

This PR applies the first two of three planned compression strategies, taking base.css from ~15.5 KB → ~ 9.77 KB.

📦 42.19.1-dev-v26385918476.1
image

Strategy 1: Drop legacy fallback declaration in bpk-themeable-property

packages/backpack-web/src/bpk-mixins/_utils.scss

Each callsite of bpk-themeable-property previously emitted two declarations:

// before — two declarations per use
@mixin bpk-themeable-property($property, $variableName, $fallback) {
  #{$property}: $fallback;
  #{$property}: var($variableName, $fallback);
}

The first declaration was a fallback for browsers without CSS custom property support (IE 11 and earlier). Backpack's browserslist targets are now Chrome 109+, Edge 142+, Firefox 145+, Safari 16+, Samsung 29+ — every supported browser implements CSS custom properties natively, so the bare-fallback line is dead weight.

// after — one declaration per use
@mixin bpk-themeable-property($property, $variableName, $fallback) {
  #{$property}: var($variableName, $fallback);
}

The SCSS-side \$fallback is preserved as the second argument to var(), so behaviour when the CSS custom property is undefined at runtime is unchanged. With ~167 callsites across central mixins (_buttons, _chips, _forms, _badges, _typography) and 19 component .module.scss files, halving these emissions is the highest-leverage byte-saving available without touching consumer call sites.

Strategy 2: Delta-only dark theme via a PostCSS dedupe plugin

scripts/webpack/postcss-dedupe-dark-theme.js (new), scripts/webpack/postCssPlugins.js, packages/backpack-web/src/bpk-stylesheets/webpack.config.babel.js

The token block emitted by Style Dictionary repeats every token under :root[data-theme=\"dark\"], including ones whose values are identical to the light theme. These duplicates are wasted bytes — the cascade would inherit the light value anyway.

A new PostCSS plugin (bpk-dedupe-dark-theme) is added that, when bundling theme-backpack-dark.css:

  1. Reads the sibling theme-backpack-light.css and parses its :root { ... } block into a Map<propName, value>.
  2. Walks the :root[data-theme=\"dark\"] rule and removes any declaration whose value matches the light theme's value for the same property.
  3. Registers the light file as a webpack dependency so base.css is rebuilt when light tokens change.

Tokens that the dark theme does override remain in place; tokens that match the light value are dropped and inherited through the CSS cascade. Visual behaviour is unchanged.

The plugin is gated by filename, so it only triggers for the bundled theme-backpack-dark.css. The standalone theme-backpack-dark.css published in the package (copied without webpack) stays complete, so consumers importing the dark theme file directly still receive every token.

A small wiring change is included alongside the plugin: webpack.config.babel.js now invokes postCssPlugins() instead of passing the function reference, so the plugin list is materialised correctly.

Scope

  • No callsite changes — bpk-themeable-property's signature is unchanged, all 167 existing usages continue to work.
  • No browser regression at Backpack's stated support targets.
  • The dark-theme dedupe applies only to the webpack-bundled base.css; standalone theme files remain complete.

What this PR does not do

  • Does not remove the SCSS-side fallback values inlined as \$fallback arguments. Those continue to flow into var(name, fallback) so consumers without base.css keep rendering.
  • Does not migrate components to the new token-sync variable names (the long-term fix discussed in the slack thread). That migration is separate epic-sized work.
  • Does not delete the bpk-themeable-property mixin itself; that depends on the migration above.
  • Does not include Strategy 3 (PostCSS class-name mangling) — that lands in a follow-up PR.

GC Zhu (gc-skyscanner) and others added 12 commits May 18, 2026 17:29
Import the three token-sync CSS files (primitives, light theme, dark
theme) into the base stylesheet so all consumers of
@Skyscanner/backpack-web/bpk-stylesheets get the design-token CSS
custom properties at runtime.

- index.scss: @use the three generated files; Sass compressed mode
  drops their license headers, so base.css keeps a single header
- README: document dark mode (set data-theme="dark" on <html>) and
  flag the variables as internal-use-only; fix two stale base.js
  references in the Contributing section
- base.css: regenerated; dark theme cascades via :root[data-theme=dark]

Note: this is the first @use 'foo.css' (Sass importing plain CSS) in
the repo. Existing @use callsites all reference SCSS. Supported since
Dart Sass 1.23; repo is on 1.90.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address review feedback on #4492:

1. Sass paths must resolve from the published package. Previously
   index.scss did `@use '../../token-sync/css/*.css'`, which works
   in-repo but breaks for consumers compiling index.scss from
   node_modules — the published tarball does not include token-sync/.
   Mirror the normalize.scss precedent: copy the three CSS files into
   packages/bpk-stylesheets/ at build time (gitignored via the
   existing *.css rule), and switch index.scss to local paths.

2. README link to ../../token-sync/ would 404 on npm. Replace with
   absolute GitHub URL.

base.css output is byte-identical to before.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Make the new build:copy-token-css npm script consistent with its
sibling build:copy-normal_css.

- scripts/scss/copy-token-css.sh: set exec bit (100755) to match
  copy-normal_css.sh
- package.json: drop the `bash` prefix and invoke directly as
  ./scripts/scss/copy-token-css.sh

Both invocations work, but the file-mode/invocation mismatch was a
latent footgun: a future contributor aligning package.json to the
sibling pattern would have hit "permission denied" on a file
committed without the exec bit.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Note in the Contributing section that index.scss depends on
gitignored copy steps (normalize + token CSS), and that running
build:stylesheets or build:sass standalone requires running the
two copy scripts first. npm run build still handles this
automatically via run-s build:*.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…bles

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previously index.scss reached into vendored token CSS via @use,
which forced build:sass (compiles all SCSS via glob) to depend on
copy:token-css and made it tricky to wire copy into build:stylesheets
without duplicating the work during npm run build.

Move the three token CSS imports out of index.scss and into
index.js (after ./index.scss) so they reach base.css through
webpack's CSS pipeline instead of sass. To keep the published
base.css from ballooning with three duplicated Apache license
blocks (one per vendored token CSS file), wire postcss-discard-comments
into the .css rule's postcss chain so headers are stripped before
WrapperPlugin re-adds the single Backpack license header.

Net effect:
- build:sass no longer needs copy:token-css.
- build:copy-token-css renamed to copy:token-css so it is no longer
  picked up by run-s build:*; build:stylesheets invokes it directly.
- npm run build runs copy:token-css exactly once (via build:stylesheets);
  npm run build:stylesheets is self-contained on a fresh checkout.

base.css size: 21,365 → 22,083 bytes raw (+3.4%), but gzipped/brotli
are slightly smaller (-30 / -28 bytes) due to more regular formatting.
README updated to reflect the new layout.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Cherry-picked from #4441 to enable publishing dev versions of this
branch for consumer-side verification.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the inline strip-comments PostCSS plugin with
css-minimizer-webpack-plugin (cssnano default preset). cssnano
strips comments and minifies the output, shrinking base.css.

The Backpack license header added by wrapper-webpack-plugin is now
emitted as a `/*!` important comment so cssnano preserves it.

base.css 22,083 -> 19,876 bytes raw, gzip 4,023 -> 3,915, brotli
3,235 -> 3,174.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- bpk-stylesheets: trim build pipeline notes to the essentials
- token-sync: surface the `build:stylesheets` step after `tokens:sync`
  so regenerated tokens make it into `base.css`

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Each callsite of bpk-themeable-property currently emits two declarations: a
bare SCSS fallback followed by the var() form, to support browsers without
CSS custom properties. Backpack's browserslist targets (Chrome 109+, Edge
142+, Firefox 145+, Safari 16+, Samsung 29+) all support CSS custom
properties, so the bare fallback is dead weight. Drop it; keep the SCSS
fallback inline inside var() so the rendered behaviour is unchanged when
the custom property is undefined.

This roughly halves the bytes emitted at every themeable-property callsite
across mixins (_buttons, _chips, _forms, _badges, _typography) and 19
component .module.scss files (~167 sites total), partially offsetting the
:root token block introduced in this PR.
IrinaWei added a commit that referenced this pull request May 25, 2026
The script hardcoded the nested workspace path
`packages/backpack-web/node_modules/normalize.css/normalize.css`. npm may
hoist `normalize.css` to the root `node_modules/` depending on install
state, in which case the nested path is missing and the build fails with:

    cp: cannot stat 'packages/backpack-web/node_modules/normalize.css/...':
    No such file or directory

Try the nested path first to preserve the existing behaviour, fall back to
the root `node_modules/normalize.css/normalize.css`, and exit with a clear
error if neither exists.

(Note: the script was introduced upstream in #4492; fixing here so #4630's
dev-release pipeline can run end-to-end. Worth porting back to that PR.)
IrinaWei added a commit that referenced this pull request May 25, 2026
The script hardcoded the nested workspace path
`packages/backpack-web/node_modules/normalize.css/normalize.css`. npm may
hoist `normalize.css` to the root `node_modules/` depending on install
state, in which case the nested path is missing and the build fails with:

    cp: cannot stat 'packages/backpack-web/node_modules/normalize.css/...':
    No such file or directory

Try the nested path first to preserve the existing behaviour, fall back to
the root `node_modules/normalize.css/normalize.css`, and exit with a clear
error if neither exists.

(Note: the script was introduced upstream in #4492; fixing here so #4630's
dev-release pipeline can run end-to-end. Worth porting back to that PR.)
@IrinaWei IrinaWei force-pushed the phase2-themeable-property-no-fallback-decl branch from a86772d to 78dd730 Compare May 25, 2026 05:54
The script hardcoded the nested workspace path
`packages/backpack-web/node_modules/normalize.css/normalize.css`. npm may
hoist `normalize.css` to the root `node_modules/` depending on install
state, in which case the nested path is missing and the build fails with:

    cp: cannot stat 'packages/backpack-web/node_modules/normalize.css/...':
    No such file or directory

Try the nested path first to preserve the existing behaviour, fall back to
the root `node_modules/normalize.css/normalize.css`, and exit with a clear
error if neither exists.

(Note: the script was introduced upstream in #4492; fixing here so #4630's
dev-release pipeline can run end-to-end. Worth porting back to that PR.)
@IrinaWei IrinaWei force-pushed the phase2-themeable-property-no-fallback-decl branch from 78dd730 to b6415d2 Compare May 25, 2026 05:56
@IrinaWei IrinaWei changed the title [bpk-mixins] Drop legacy fallback declaration in bpk-themeable-property [CLOV-1637] Phase 1: Drop legacy fallback declaration in bpk-themeable-property and delete duplicate dark style May 25, 2026
Base automatically changed from gc-skyscanner/clov-1412 to main May 29, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants