From 439efc7419d76d6e8d7e816d7533abd547faa400 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 1 Sep 2026 11:34:46 +0100 Subject: [PATCH] DEV: Introduce system for renaming CSS variables (#42733) From time-to-time, variables may need to be renamed in core. To avoid breaking existing themes and plugins, this commit introduces a system to automatically rewrite old variable names to new ones. A list of renames is maintained in `stylesheets/variable-renames.json`, and is applied when CSS is compiled. This list also powers a stylelint rule which will automatically rewrite old names to new names in source code. Transformations apply to all core/theme/plugin code. For now, the stylelint rule/autofix applies to core only, but this will be extracted to `@discourse/lint-configs` in the near future. When a transformation is applied, it adds a trailing `/* automatically renamed --old to --new */` comment so that the behavior is understandable from the browser developer tools. --- .../references/color-and-theming.md | 7 ++ app/assets/stylesheets/variable-renames.json | 1 + frontend/asset-processor/package.json | 1 + .../postcss-variable-renamer.js | 56 ++++++++++++++ .../postcss-variable-renamer.test.mjs | 77 +++++++++++++++++++ frontend/asset-processor/postcss.js | 2 + lib/asset_processor.rb | 1 + lib/stylesheet/manager.rb | 2 + lib/stylesheet/manager/builder.rb | 2 +- package.json | 1 + pnpm-lock.yaml | 6 ++ spec/lib/stylesheet/manager_spec.rb | 22 ++++++ stylelint-rules/no-renamed-variables.mjs | 60 +++++++++++++++ stylelint.config.mjs | 9 ++- 14 files changed, 245 insertions(+), 2 deletions(-) create mode 100644 app/assets/stylesheets/variable-renames.json create mode 100644 frontend/asset-processor/postcss-variable-renamer.js create mode 100644 frontend/asset-processor/postcss-variable-renamer.test.mjs create mode 100644 stylelint-rules/no-renamed-variables.mjs diff --git a/.skills/discourse-writing-html-css/references/color-and-theming.md b/.skills/discourse-writing-html-css/references/color-and-theming.md index 22ce54b4ba1..25c73f0ae20 100644 --- a/.skills/discourse-writing-html-css/references/color-and-theming.md +++ b/.skills/discourse-writing-html-css/references/color-and-theming.md @@ -101,3 +101,10 @@ these to restyle every button of a variant without touching `.btn` selectors. `--d-sidebar-animation-time`/`-ease`, etc. The admin layout's sidebar adds `--d-sidebar-admin-*` (e.g. `--d-sidebar-admin-background`) in `admin/sidebar.scss`. Override these to retheme the sidebar in either context without overriding its internal selectors. + +## Renaming variables + +From time-to-time, variables may need to be renamed in core. To avoid breaking existing themes and plugins, +Discourse will automatically rewrite old variable names to new ones. A list of renames is maintained in +`stylesheets/variable-renames.json`, and is applied when CSS is compiled. This list also powers +a stylelint rule which will automatically rewrite old names to new names in source code. diff --git a/app/assets/stylesheets/variable-renames.json b/app/assets/stylesheets/variable-renames.json new file mode 100644 index 00000000000..0967ef424bc --- /dev/null +++ b/app/assets/stylesheets/variable-renames.json @@ -0,0 +1 @@ +{} diff --git a/frontend/asset-processor/package.json b/frontend/asset-processor/package.json index ea8f2e370b7..4bfed43d79e 100644 --- a/frontend/asset-processor/package.json +++ b/frontend/asset-processor/package.json @@ -32,6 +32,7 @@ "polyfill-crypto.getrandomvalues": "^1.0.0", "postcss": "^8.5.15", "postcss-nesting": "^14.0.0", + "postcss-value-parser": "^4.2.0", "rolldown": "1.1.2", "source-map-js": "^1.2.1", "strip-test-selectors": "^0.1.0", diff --git a/frontend/asset-processor/postcss-variable-renamer.js b/frontend/asset-processor/postcss-variable-renamer.js new file mode 100644 index 00000000000..10e78329931 --- /dev/null +++ b/frontend/asset-processor/postcss-variable-renamer.js @@ -0,0 +1,56 @@ +import valueParser from "postcss-value-parser"; +import renames from "../../app/assets/stylesheets/variable-renames.json"; + +/** + * Rewrites renamed CSS custom properties to their current names, so + * stylesheets which set or read an old name keep working. Each rewritten + * declaration gets a trailing `automatically renamed` comment as a clue in + * dev-tools. + */ + +export default function postcssVariableRenamer(renameMap = renames) { + const oldNames = Object.keys(renameMap); + + return { + postcssPlugin: "postcss-variable-renamer", + + Declaration(declaration, { Comment }) { + const applied = new Map(); + + const newProp = renameMap[declaration.prop]; + if (newProp) { + applied.set(declaration.prop, newProp); + declaration.prop = newProp; + } + + if (oldNames.some((oldName) => declaration.value.includes(oldName))) { + let valueChanged = false; + const parsed = valueParser(declaration.value); + + parsed.walk((node) => { + const newName = node.type === "word" && renameMap[node.value]; + if (newName) { + applied.set(node.value, newName); + node.value = newName; + valueChanged = true; + } + }); + + if (valueChanged) { + declaration.value = parsed.toString(); + } + } + + if (applied.size > 0) { + const details = [...applied] + .map(([oldName, newName]) => `${oldName} to ${newName}`) + .join(", "); + declaration.after( + new Comment({ text: `automatically renamed ${details}` }) + ); + } + }, + }; +} + +postcssVariableRenamer.postcss = true; diff --git a/frontend/asset-processor/postcss-variable-renamer.test.mjs b/frontend/asset-processor/postcss-variable-renamer.test.mjs new file mode 100644 index 00000000000..0fb4c84f9b9 --- /dev/null +++ b/frontend/asset-processor/postcss-variable-renamer.test.mjs @@ -0,0 +1,77 @@ +/* eslint-disable qunit/require-expect */ +import postcss from "postcss"; +import { expect, test } from "vitest"; +import renames from "../../app/assets/stylesheets/variable-renames.json"; +import postcssVariableRenamer from "./postcss-variable-renamer.js"; + +const testMap = { "--old-name": "--new-name" }; + +function process(css) { + return postcss([postcssVariableRenamer(testMap)]).process(css, { + from: undefined, + }).css; +} + +test("rewrites declarations of a renamed variable", () => { + expect(process(":root { --old-name: red; }")).toBe( + ":root { --new-name: red; /* automatically renamed --old-name to --new-name */ }" + ); +}); + +test("rewrites var() references", () => { + expect(process("a { color: var(--old-name); }")).toBe( + "a { color: var(--new-name); /* automatically renamed --old-name to --new-name */ }" + ); +}); + +test("rewrites references inside fallbacks", () => { + expect(process("a { color: var(--brand, var(--old-name)); }")).toBe( + "a { color: var(--brand, var(--new-name)); /* automatically renamed --old-name to --new-name */ }" + ); +}); + +test("preserves fallback of a renamed reference", () => { + expect(process("a { color: var(--old-name, blue); }")).toBe( + "a { color: var(--new-name, blue); /* automatically renamed --old-name to --new-name */ }" + ); +}); + +test("leaves longer names alone", () => { + const css = "a { color: var(--old-name-hover); }"; + expect(process(css)).toBe(css); +}); + +test("leaves names with a prefix alone", () => { + const css = "a { color: var(--theme--old-name); }"; + expect(process(css)).toBe(css); +}); + +test("adds one comment when a declaration sets and reads the same old name", () => { + expect(process(":root { --old-name: var(--old-name, red); }")).toBe( + ":root { --new-name: var(--new-name, red); /* automatically renamed --old-name to --new-name */ }" + ); +}); + +test("leaves untouched declarations alone", () => { + const css = "a { color: var(--primary); }"; + expect(process(css)).toBe(css); +}); + +test("leaves string contents alone", () => { + const css = 'a::before { content: "--old-name"; }'; + expect(process(css)).toBe(css); +}); + +test("map has no chains", () => { + const oldNames = new Set(Object.keys(renames)); + for (const newName of Object.values(renames)) { + expect(oldNames).not.toContain(newName); + } +}); + +test("map only contains custom property names", () => { + for (const [oldName, newName] of Object.entries(renames)) { + expect(oldName).toMatch(/^--[\w-]+$/); + expect(newName).toMatch(/^--[\w-]+$/); + } +}); diff --git a/frontend/asset-processor/postcss.js b/frontend/asset-processor/postcss.js index b6051df72f4..49945911db3 100644 --- a/frontend/asset-processor/postcss.js +++ b/frontend/asset-processor/postcss.js @@ -5,8 +5,10 @@ import postcss from "postcss"; import postcssNesting from "postcss-nesting"; import { browsers } from "../discourse/config/targets"; import postcssVariablePrefixer from "./postcss-variable-prefixer"; +import postcssVariableRenamer from "./postcss-variable-renamer"; const postCssProcessor = postcss([ + postcssVariableRenamer(), autoprefixer({ overrideBrowserslist: browsers, }), diff --git a/lib/asset_processor.rb b/lib/asset_processor.rb index 0c602a85220..9dfe17f9ee4 100644 --- a/lib/asset_processor.rb +++ b/lib/asset_processor.rb @@ -10,6 +10,7 @@ class AssetProcessor dependency_globs: %w[ node_modules/.pnpm/lock.yaml frontend/asset-processor/**/*.{js,mjs} + app/assets/stylesheets/variable-renames.json frontend/discourse/lib/babel-transform-module-renames.js frontend/discourse/lib/discourse-source-imports.mjs frontend/discourse/config/targets.js diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb index 96abefbb7d5..1e4c2b322f1 100644 --- a/lib/stylesheet/manager.rb +++ b/lib/stylesheet/manager.rb @@ -17,6 +17,7 @@ class Stylesheet::Manager private_constant :CACHE_PATH MANIFEST_DIR = "#{Rails.root.join("tmp/cache/assets/#{Rails.env}")}" + VARIABLE_RENAMES_PATH = Rails.root.join("app/assets/stylesheets/variable-renames.json") THEME_REGEX = /_theme(_rtl)?\z/ COLOR_SCHEME_STYLESHEET = "color_definitions" @@ -189,6 +190,7 @@ class Stylesheet::Manager "#{Rails.root.join("app/assets/stylesheets/**/*.*css")}", "#{Rails.root.join("app/assets/images/**/*.*")}", "#{Rails.root.join("lib/stylesheet/*.rb")}", + VARIABLE_RENAMES_PATH.to_s, ] Discourse.plugins.each do |plugin| diff --git a/lib/stylesheet/manager/builder.rb b/lib/stylesheet/manager/builder.rb index 57d2be21848..8618298aab5 100644 --- a/lib/stylesheet/manager/builder.rb +++ b/lib/stylesheet/manager/builder.rb @@ -193,7 +193,7 @@ class Stylesheet::Manager::Builder def theme_digest Digest::SHA1.hexdigest( scss_digest.to_s + color_scheme_digest.to_s + settings_digest + uploads_digest + - current_hostname, + current_hostname + Stylesheet::Manager.fs_asset_cachebuster, ) end diff --git a/package.json b/package.json index b95577c7dd4..446d3f0357d 100644 --- a/package.json +++ b/package.json @@ -21,6 +21,7 @@ "playwright": "1.59.1", "postcss-selector-parser": "^7.1.4", "prettier": "3.8.1", + "postcss-value-parser": "^4.2.0", "stylelint": "17.5.0", "typescript": "^5.9.3" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 907e01b20c9..fe1ca6499f4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -90,6 +90,9 @@ importers: postcss-selector-parser: specifier: ^7.1.4 version: 7.1.4 + postcss-value-parser: + specifier: ^4.2.0 + version: 4.2.0 prettier: specifier: 3.8.1 version: 3.8.1 @@ -177,6 +180,9 @@ importers: postcss-nesting: specifier: ^14.0.0 version: 14.0.0(postcss@8.5.15) + postcss-value-parser: + specifier: ^4.2.0 + version: 4.2.0 rolldown: specifier: 1.1.2 version: 1.1.2 diff --git a/spec/lib/stylesheet/manager_spec.rb b/spec/lib/stylesheet/manager_spec.rb index f7308b0bdf6..d6d6c3a923c 100644 --- a/spec/lib/stylesheet/manager_spec.rb +++ b/spec/lib/stylesheet/manager_spec.rb @@ -317,6 +317,22 @@ RSpec.describe Stylesheet::Manager do expect(digest1).not_to eq(digest2) end + it "accounts for the asset cachebuster in theme digests" do + theme = Fabricate(:theme) + + builder = + Stylesheet::Manager::Builder.new(target: :desktop_theme, theme: theme, manager: manager) + digest1 = builder.digest + + Stylesheet::Manager.stubs(:fs_asset_cachebuster).returns("changed") + + builder = + Stylesheet::Manager::Builder.new(target: :desktop_theme, theme: theme, manager: manager) + digest2 = builder.digest + + expect(digest1).not_to eq(digest2) + end + it "can correctly account for settings in theme's components" do theme = Fabricate(:theme) child = Fabricate(:theme, component: true) @@ -1130,5 +1146,11 @@ RSpec.describe Stylesheet::Manager do expect(new_cachebuster).not_to eq(initial_cachebuster) end end + + it "includes the variable rename map in its inputs" do + expect(Stylesheet::Manager.send(:list_files)).to include( + Stylesheet::Manager::VARIABLE_RENAMES_PATH.to_s, + ) + end end end diff --git a/stylelint-rules/no-renamed-variables.mjs b/stylelint-rules/no-renamed-variables.mjs new file mode 100644 index 00000000000..178358d7f4e --- /dev/null +++ b/stylelint-rules/no-renamed-variables.mjs @@ -0,0 +1,60 @@ +import { readFileSync } from "node:fs"; +import valueParser from "postcss-value-parser"; +import stylelint from "stylelint"; + +const renames = JSON.parse( + readFileSync( + new URL("../app/assets/stylesheets/variable-renames.json", import.meta.url) + ) +); + +const ruleName = "discourse/no-renamed-variables"; +const messages = stylelint.utils.ruleMessages(ruleName, { + renamed: (oldName, newName) => `"${oldName}" was renamed to "${newName}"`, +}); + +const ruleFunction = (primaryOption) => { + return (root, result) => { + if (!primaryOption) { + return; + } + + root.walkDecls((decl) => { + const newProp = renames[decl.prop]; + if (newProp) { + stylelint.utils.report({ + message: messages.renamed(decl.prop, newProp), + node: decl, + result, + ruleName, + word: decl.prop, + fix: () => (decl.prop = newProp), + }); + } + + const parsed = valueParser(decl.value); + parsed.walk((node) => { + const newName = node.type === "word" && renames[node.value]; + if (newName) { + stylelint.utils.report({ + message: messages.renamed(node.value, newName), + node: decl, + result, + ruleName, + word: node.value, + fix: () => { + node.value = newName; + decl.value = parsed.toString(); + }, + }); + } + }); + }); + }; +}; + +ruleFunction.ruleName = ruleName; +ruleFunction.messages = messages; +ruleFunction.meta = { fixable: true }; + +export default stylelint.createPlugin(ruleName, ruleFunction); diff --git a/stylelint.config.mjs b/stylelint.config.mjs index 42361336a34..f892569d60a 100644 --- a/stylelint.config.mjs +++ b/stylelint.config.mjs @@ -1,12 +1,19 @@ import noCoreVariables from "./stylelint-rules/no-core-variables.mjs"; +import noRenamedVariables from "./stylelint-rules/no-renamed-variables.mjs"; import requireDesignTokens from "./stylelint-rules/require-design-tokens.mjs"; import ucClassesInWhere from "./stylelint-rules/uc-classes-in-where.mjs"; export default { extends: ["@discourse/lint-configs/stylelint"], - plugins: [noCoreVariables, requireDesignTokens, ucClassesInWhere], + plugins: [ + noCoreVariables, + noRenamedVariables, + requireDesignTokens, + ucClassesInWhere, + ], rules: { "media-feature-range-notation": "context", + "discourse/no-renamed-variables": true, "discourse/uc-classes-in-where": true, }, overrides: [