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

Generating many styles with identifiers: "short", triple & selector (&&&), in a file whose hash starts with a letter in base36 leads to compilation errors #1501

Closed
2 tasks done
calebmer opened this issue Nov 8, 2024 · 2 comments · Fixed by #1505
Labels
css Issue related to the core css package

Comments

@calebmer
Copy link

calebmer commented Nov 8, 2024

Describe the bug

Say you have a bug-repro.css.ts file that looks like this:

import {style} from "@vanilla-extract/css";

export const classNames: Array<string> = [];

for (let i = 0; i < 3000; i++) {
    classNames.push(style({width: i, selectors: {"&&&": {height: i}}}));
}

(I use the selector &&& as an easy way to surgically increase the specificity of some CSS rule instead of using the cudgel !important.)

On my machine the file hash for this is lfv1o4 (which seems to be generated here). So class names follow the pattern:

lfv1o40
lfv1o41
lfv1o42
lfv1o43
...

At i = 93 we generate the class lfv1o42l. Where lfv1o4 is the file hash and 2l is the base36 encoded ref count.

// Convert ref count to base 36 for optimal hash lengths
const refCount = getAndIncrementRefCounter().toString(36);
const { filePath, packageName } = getFileScope();
const fileScopeHash = hash(
packageName ? `${packageName}${filePath}` : filePath,
);
let identifier = `${fileScopeHash}${refCount}`;

When @vanilla-extract generates the selector &&& for lfv1o42 (our class name at i = 2) it generates the string lfv1o42lfv1o42lfv1o42. Then Stylesheet.transformSelector() is responsible for transforming it into the actual selector we'll add to the CSS file. Which should be .lfv1o42.lfv1o42.lfv1o42.

transformSelector(selector: string) {
// Map class list compositions to single identifiers
let transformedSelector = selector;
for (const { identifier, regex } of this.composedClassLists) {
transformedSelector = transformedSelector.replace(regex, () => {
markCompositionUsed(identifier);
return identifier;
});
}
if (this.localClassNamesMap.has(transformedSelector)) {
return this.transformClassname(transformedSelector);
}
const results = this.localClassNamesSearch.search(transformedSelector);
let lastReplaceIndex = transformedSelector.length;
// Perform replacements backwards to simplify index handling
for (let i = results.length - 1; i >= 0; i--) {
const [endIndex, [firstMatch]] = results[i];
const startIndex = endIndex - firstMatch.length + 1;
if (startIndex >= lastReplaceIndex) {
// Class names can be substrings of other class names
// e.g. '_1g1ptzo1' and '_1g1ptzo10'
// If the startIndex >= lastReplaceIndex, then
// this is the case and this replace should be skipped
continue;
}
lastReplaceIndex = startIndex;
// If class names already starts with a '.' then skip
if (transformedSelector[startIndex - 1] !== '.') {
transformedSelector = replaceBetweenIndexes(
transformedSelector,
startIndex,
endIndex + 1,
this.transformClassname(firstMatch),
);
}
}
return transformedSelector;
}

The problem is, it's ambiguous how to transform this class name! Given both lfv1o42 (at i = 2) and lfv1o402l (at i = 93) are valid class names. @vanilla-extract resolves this by always picking the longest match:

if (startIndex >= lastReplaceIndex) {
// Class names can be substrings of other class names
// e.g. '_1g1ptzo1' and '_1g1ptzo10'
// If the startIndex >= lastReplaceIndex, then
// this is the case and this replace should be skipped
continue;
}

…but that means in lfv1o42lfv1o42lfv1o42 (from the selector &&&) is parsed such that lfv1o42lf is considered to be the first class name. In my local example I get the following error:

Pre-transform error: Invalid selector: &lffv1o42lffv1o42

Style selectors must target the '&' character (along with any modifiers), e.g. `${parent} &` or
`{parent} &:hover`.

This is to ensure that each style block only affects the styling of a single class.

If your selector is targeting another class, you should move it to the style definition for that
class, e.g. given we have styles for 'parent' and 'child' elements, instead of adding a selector of
`& ${child}`) to 'parent', you should add `${parent} &` to 'child').

If your selector is targeting something global, use the 'globalStyle' function instead, e.g. if you
wanted to write `& h1`, you should instead write 'globalStyle(`${parent} h1`, { ... })'

Clearly something is broken with how @vanilla-extract is parsing the class name then printing it back out if it thinks my selector &&& is actually &lffv1o42lffv1o42.

This issue is non-deterministic. It doesn't effect files who hash to a value starting with a number (e.g. I have one CSS file in production whose hash is 93u2u5). Since normalizeIdentifier() prefixes classes that start with a number with an _. Because an _ is not a base36 character (ref counts in class names are serialized to base36) we never run into this ambiguous case. Since in a class name with a hash of 93u2u5, ref count of 2, we get _93u2u52. We'll never generate another class name that ends with an underscore so @vanilla-extract's string searching won't be confused.

function normalizeIdentifier(identifier: string) {
return identifier.match(/^[0-9]/) ? `_${identifier}` : identifier;
}

This was a very annoying issue to debug. It was happening in my production deploy CI job but wasn't happening on local developer machines or my test CI job (even when identifiers were set to short). I thought this was Linux related, then arm64 vs x86_64 related, and turns out it's based on how the file name is hashed.

Replacing identifiers: "short" with identifiers: ({hash}) => hash.startsWith("_") ? `_${hash}` : hash works around the issue by making sure all classes start with an underscore. Filing this issue so either this fix can be added to @vanilla-extract itself or a more principled fix can be written up.

Reproduction

Apologies for not providing a minimal reproduction. I created one locally with Vite but since this issue is based on how the absolute path file is hashed I figured it might be better to only include the essential part of the reproduction (the bug-repro.css.ts file above) and let maintainers iterate on their own reproduction based on the absolute file paths on their machine.

System Info

System:
    OS: macOS 14.2.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 114.42 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.9.0 - /private/var/tmp/_bazel_calebmer/bdcb08d3184e81b525008307e7ae9d3a/external/node_darwin_arm64/bin/node
    npm: 10.1.0 - /private/var/tmp/_bazel_calebmer/bdcb08d3184e81b525008307e7ae9d3a/external/node_darwin_arm64/bin/npm
    pnpm: 8.15.3 - ~/Projects/cyberworlds/admin/bin/pnpm
  Browsers:
    Chrome: 130.0.6723.117
    Safari: 17.2.1
  npmPackages:
    @vanilla-extract/vite-plugin: ^4.0.17 => 4.0.17
    vite: ^5.4.10 => 5.4.10

Used Package Manager

npm

Logs

Here's the original error message I was seeing in CI. Clearly &&& is a valid selector:

✘ [ERROR] Invalid selector: &&&

Style selectors must target the '&' character (along with any modifiers), e.g. `${parent} &` or `${parent} &:hover`.

This is to ensure that each style block only affects the styling of a single class.

If your selector is targeting another class, you should move it to the style definition for that class, e.g. given we have styles for 'parent' and 'child' elements, instead of adding a selector of `& ${child}`) to 'parent', you should add `${parent} &` to 'child').

If your selector is targeting something global, use the 'globalStyle' function instead, e.g. if you wanted to write `& h1`, you should instead write 'globalStyle(`${parent} h1`, { ... })' [plugin vanilla-extract]

Validations

@askoufis
Copy link
Contributor

Thank you so much for the very detailed bug report! This is a tricky one. I have a reproduction in a test application, as well as in a unit test for transformCss, but I'm not quite sure on a good solution just yet.

@askoufis
Copy link
Contributor

#1505 should fix your issue. You can test out this version if you want to confirm that the patch fixes the issue in your repo: @vanilla-extract/css@fix-buggy-selector-transform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css Issue related to the core css package
Projects
None yet
2 participants