Skip to content
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

Ensure namespace reset with escaped * works #15603

Merged
merged 5 commits into from
Jan 13, 2025

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Jan 11, 2025

This PR fixes an issue if you want to reset a namespace like:

@theme {
  --color-*: initial;
}

That some formatters such as Biome won't allow this syntax. To solve that, this PR allows you to use an escaped * character.

@theme {
  --color-\*: initial;
}

Fixes: #15602

@RobinMalfait RobinMalfait requested a review from a team as a code owner January 11, 2025 19:05
@RobinMalfait RobinMalfait changed the title Ensure a namespace reset such as --color-\*: initial works Ensure namespace reset with escaped * works Jan 11, 2025
@@ -41,6 +41,10 @@ export class Theme {
) {}

add(key: string, value: string, options = ThemeOptions.NONE): void {
if (key.endsWith('-\\*')) {
key = key.replace(/-\\\*$/, '-*')
}
Copy link
Contributor

@thecrypticace thecrypticace Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a significant penalty to just always running the regex replace? Since it's anchored I'd expect it to not be too bad (maybe hoist the regex itself tho?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep it's faster with the regex if you hoist it up!

Benchmark 1: regex-always
  Time (mean ± σ):     322.9 ms ±   8.8 ms    [User: 1180.2 ms, System: 115.0 ms]
  Range (min … max):   312.5 ms … 341.1 ms    10 runs

Benchmark 2: regex-conditional
  Time (mean ± σ):     329.7 ms ±  10.7 ms    [User: 1235.7 ms, System: 139.1 ms]
  Range (min … max):   317.5 ms … 349.8 ms    10 runs

Summary
  regex-always ran
    1.02 ± 0.04 times faster than regex-conditional

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if you don't use a regex at all and just use key.slice(0, -2) + '*' to build a new string, since you know the number of characters you are removing from the end?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I did a microbenchmark of the slicing:

import { run, bench } from "mitata";

function slice1(key) {
  let escapedStarIdx = key.lastIndexOf("-\\*");
  if (escapedStarIdx !== -1) {
    key = key.slice(0, escapedStarIdx) + "-*";
  }

  return key;
}

function slice2(key) {
  if (key.endsWith("-\\*")) {
    key = key.slice(0, -3) + "-*";
  }

  return key;
}

let regex = /-\\\*$/;
function slice3(key) {
  return key.replace(regex, "-*");
}

let keys = ["--color-\\*", "--font-\\*", "--border-\\*", "--background-\\*", "--padding-\\*"];

let n = 0;

bench("slice1(…)", () => slice1(keys[(n = (n + 1) % 5)]));
bench("slice2(…)", () => slice2(keys[(n = (n + 1) % 5)]));
bench("slice3(…)", () => slice3(keys[(n = (n + 1) % 5)]));

await run();
clk: ~3.77 GHz
cpu: Apple M3 Max
runtime: node 23.6.0 (arm64-darwin)

benchmark                   avg (min … max) p75 / p99    (min … top 1%)
------------------------------------------- -------------------------------
slice1(…)                     15.06 ns/iter  14.94 ns   █                  
                      (13.75 ns … 42.47 ns)  24.37 ns   █                  
                    ( 27.20 kb …  62.53 kb)  27.20 kb ▁▁█▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
slice2(…)                      8.07 ns/iter   7.95 ns █                    
                       (7.79 ns … 50.43 ns)  18.65 ns █                    
                    ( 27.21 kb …  62.18 kb)  27.18 kb █▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
slice3(…)                     34.29 ns/iter  34.11 ns   █                  
                      (32.49 ns … 67.69 ns)  47.01 ns   █                  
                    ( 57.61 kb …  61.75 kb)  56.79 kb ▁▁█▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
clk: ~3.95 GHz
cpu: Apple M3 Max
runtime: bun 1.1.43 (arm64-darwin)

benchmark                   avg (min … max) p75 / p99    (min … top 1%)
------------------------------------------- -------------------------------
slice1(…)                     17.00 ns/iter  16.95 ns      █               
                      (15.36 ns … 67.54 ns)  20.26 ns      █▅              
                    (  0.00  b …  84.00 kb) 142.42  b ▂▂▂▁▁██▅▃▂▂▂▂▂▂▁▁▁▁▁▁
slice2(…)                     16.66 ns/iter  16.74 ns      █               
                      (14.72 ns … 59.11 ns)  20.84 ns     ▂██              
                    (  0.00  b …  16.00 kb)  16.16  b ▁▄▆▄████▅▃▂▂▂▂▂▁▁▁▁▁▁
slice3(…)                     41.57 ns/iter  42.57 ns       ▃▄█▂           
                      (37.54 ns … 96.04 ns)  48.33 ns  ▅▂   ████▄          
                    (  0.00  b …  80.00 kb)  13.81 kb ▂██▅▄▃██████▅▄▃▂▂▂▁▁▁

Some additional testing (not shown here) shows that with a constant value Bun hyper-optimizes the regex into picoseconds but when you start passing various things through that optimization disappears. Node doesn't appear to have a similar thing here afaict.

I think this implementation is the best to use based on these numbers:

if (key.endsWith("-\\*")) {
  key = key.slice(0, -3) + "-*";
}

Copy link
Contributor

@thecrypticace thecrypticace Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err, this actually like Adam mentioned above. Don't need to slice out the - and then copy it back in lol.

if (key.endsWith("\\*")) {
  key = key.slice(0, -2) + "*";
}

@RobinMalfait RobinMalfait force-pushed the fix/allow-escaped-namespace-reset branch from 28cdc21 to 23479d1 Compare January 13, 2025 10:39
@RobinMalfait RobinMalfait enabled auto-merge (squash) January 13, 2025 10:39
@RobinMalfait RobinMalfait merged commit 3b03395 into next Jan 13, 2025
5 checks passed
@RobinMalfait RobinMalfait deleted the fix/allow-escaped-namespace-reset branch January 13, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v4] Namespace override syntax is not valid CSS
4 participants