Skip to content

Fix token fallback plugins rewriting var() inside string literals - #82209

Closed
dilipom13 wants to merge 2 commits into
WordPress:trunkfrom
dilipom13:fix/82169-token-fallback-string-literals
Closed

Fix token fallback plugins rewriting var() inside string literals#82209
dilipom13 wants to merge 2 commits into
WordPress:trunkfrom
dilipom13:fix/82169-token-fallback-string-literals

Conversation

@dilipom13

Copy link
Copy Markdown
Contributor

What

Fixes #82169

PostCSS, esbuild, and Vite token-fallback plugins were incorrectly rewriting var(--wpds-*) inside quoted CSS strings and JS comments. They now skip those regions, matching Lightning CSS behavior.

How to test

  • npm run test:unit -- --testPathPatterns=packages/theme/postcss-plugins/test/add-fallback-to-var
  • npm run test:unit -- --testPathPatterns=packages/theme/src/test/build-plugin-parity

Skip quoted CSS strings and JS comments when injecting --wpds-* fallbacks so PostCSS, esbuild, and Vite match Lightning CSS behavior.

Co-authored-by: Cursor <cursoragent@cursor.com>
@dilipom13
dilipom13 requested a review from a team as a code owner August 29, 2026 10:07
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: dilipom13 <dilip2615@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: shail-mehta <shailu25@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: simison <simison@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions Bot added the [Package] Theme /packages/theme label Aug 29, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
@shail-mehta

Copy link
Copy Markdown
Member

@dilipom13 Thanks for the PR!

Just a quick note: Good First Issues are intended to help new WordPress contributors get started, so they’re generally reserved for new contributors.

@shail-mehta shail-mehta added the [Type] Bug An existing feature does not function as intended label Aug 29, 2026
@dilipom13

Copy link
Copy Markdown
Contributor Author

@dilipom13 Thanks for the PR!

Just a quick note: Good First Issues are intended to help new WordPress contributors get started, so they’re generally reserved for new contributors.

Okay @shail-mehta

Thanks for your appreciation.

Comment on lines +90 to +119
if ( char === '/' && next === '/' ) {
const end = source.indexOf( '\n', index );
const sliceEnd = end === -1 ? source.length : end;
result += source.slice( index, sliceEnd );
index = sliceEnd;
continue;
}

if ( char === '/' && next === '*' ) {
const end = source.indexOf( '*/', index + 2 );
const sliceEnd = end === -1 ? source.length : end + 2;
result += source.slice( index, sliceEnd );
index = sliceEnd;
continue;
}

if ( char === '"' || char === "'" ) {
const end = readQuotedSegment( source, index );
const literal = source.slice( index, end );
const quote = literal[ 0 ];
const inner = literal.slice( 1, -1 );
const transformed = transformCssValue(
inner,
tokenFallbacks,
options
);
result += quote + transformed + quote;
index = end;
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This scanner treats quote and comment markers inside regular-expression literals as JavaScript syntax.

For example, const re = /["']/; const s = 'var(--wpds-border-radius-sm)'; becomes invalid JavaScript.

Maybe we should use a real JS/TS parser or lexer to locate string and template contents and add regex-literal coverage?

Comment on lines +189 to +194
if ( source[ index ] === '$' && source[ index + 1 ] === '{' ) {
const expressionEnd = readTemplateExpression( source, index + 2 );
content += source.slice( index, expressionEnd );
index = expressionEnd;
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copying each template expression unchanged means CSS strings nested inside it no longer receive fallbacks. Please keep transforming string and template literals inside expressions and add a regression test for a conditional CSS value.

Example of a regression

This input was transformed at the base revision but is left unchanged at this head:

const styles = `${ enabled ? 'var(--wpds-border-radius-sm)' : '' }`;

The published esbuild and Vite plugins support JS/TS source, and @wordpress/build enables the esbuild plugin automatically when @wordpress/theme is installed. Leaving this reference bare defeats the fallback plugin when the design-token stylesheet is unavailable.

@ciampo

ciampo commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Also, this PR feels a bit overcomplicated / overengineered. Potentially a cleaner design would be:

  • A JS/TS tokenizer or parser to locate strings and template segments.
  • A CSS value parser to find real, bare var(--wpds-*) functions.
  • A small shared helper for fallback lookup and formatting.
  • Separate CSS and JS entry points instead of using escapeQuotes to switch languages.

Although let's wait for feedback from @WordPress/gutenberg-components and @manzoorwanijk before proceeding.

@mirka

mirka commented Aug 31, 2026

Copy link
Copy Markdown
Member

I added my thoughts on the original issue, as there doesn't seem to be an assessment of the cost/benefit. (Not necessarily against it, but just saying that it should be a reasoned decision.)

@simison

simison commented Aug 31, 2026

Copy link
Copy Markdown
Member

+1 as of current state, not really worth complicating this much. If there's a simple elegant solution, then it would make sense.

@ciampo

ciampo commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Right, let's close this PR for now, since there is a general agreement that the solution complexity isn't worth the benefits.

@dilipom13 , thank you for working on this anyway! In the future, probably better to coordinate on the original issue before working on a PR.

@ciampo ciampo closed this Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Theme /packages/theme [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PostCSS/esbuild/Vite token-fallback plugins incorrectly rewrite var()-like text inside strings

5 participants