-
Notifications
You must be signed in to change notification settings - Fork 4.8k
ESLint: Broaden no-unknown-ds-tokens to all strings and catch dynamic construction
#75905
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
Changes from all commits
4b567c5
30f0cc1
de4a043
5773294
1d70dd6
8f0605b
2df9954
ddd7fd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
|
|
||
| When using Design System tokens (CSS custom properties beginning with `--wpds-`), only valid public tokens should be used. Using non-existent tokens will result in broken styles since the CSS variable won't resolve to any value. | ||
|
|
||
| This rule lints JSX inline styles. For CSS files, use the [corresponding Stylelint rule](https://developer.wordpress.org/block-editor/reference-guides/packages/packages-theme/#stylelint-plugins) from the `@wordpress/theme` package. | ||
| Additionally, token names must not be dynamically constructed (e.g. via template literal expressions), as they cannot be statically verified for correctness or processed automatically to inject fallbacks. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered separating the two lint rules, but since a dynamically generated token will always evade an unknown tokens lint, I think it makes sense to keep the two lints as a single rule. The fallback injection thing is secondary — not necessarily the main motivator of this additional check. |
||
|
|
||
| This rule lints all string literals and template literals in JavaScript/TypeScript files. For CSS files, use the [corresponding Stylelint rule](https://developer.wordpress.org/block-editor/reference-guides/packages/packages-theme/#stylelint-plugins) from the `@wordpress/theme` package. | ||
|
|
||
| ## Rule details | ||
|
|
||
|
|
@@ -12,16 +14,29 @@ Examples of **incorrect** code for this rule: | |
| <div style={ { color: 'var(--wpds-nonexistent-token)' } } /> | ||
| ``` | ||
|
|
||
| ```js | ||
| const token = 'var(--wpds-nonexistent-token)'; | ||
| ``` | ||
|
|
||
| ```jsx | ||
| <div style={ { color: 'var(--wpds-fake-color, var(--wpds-also-fake))' } } /> | ||
| ``` | ||
|
|
||
| ```js | ||
| // Dynamically constructed token names are not allowed. | ||
| const token = `var(--wpds-dimension-gap-${ size })`; | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```jsx | ||
| <div style={ { color: 'var(--wpds-color-fg-content-neutral)' } } /> | ||
| ``` | ||
|
|
||
| ```js | ||
| const token = 'var(--wpds-color-fg-content-neutral)'; | ||
| ``` | ||
|
|
||
| ```jsx | ||
| <div style={ { color: 'var(--my-custom-prop)' } } /> | ||
| ``` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,13 +27,28 @@ ruleTester.run( 'no-unknown-ds-tokens', rule, { | |
| { | ||
| code: '<div style={ { color: `var(--wpds-color-fg-content-neutral)` } } />', | ||
| }, | ||
| { | ||
| code: `const token = 'var(--wpds-color-fg-content-neutral)';`, | ||
| }, | ||
| { | ||
| code: `const name = 'something--wpds-color';`, | ||
| }, | ||
| { | ||
| code: '`${ prefix }: var(--wpds-color-fg-content-neutral)`', | ||
| }, | ||
| { | ||
| code: '`var(--wpds-color-fg-content-neutral) ${ suffix }`', | ||
| }, | ||
| ], | ||
| invalid: [ | ||
| { | ||
| code: `<div style={ { color: 'var(--wpds-nonexistent-token)' } } />`, | ||
| errors: [ | ||
| { | ||
| messageId: 'onlyKnownTokens', | ||
| data: { | ||
| tokenNames: "'--wpds-nonexistent-token'", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
|
|
@@ -53,6 +68,77 @@ ruleTester.run( 'no-unknown-ds-tokens', rule, { | |
| errors: [ | ||
| { | ||
| messageId: 'onlyKnownTokens', | ||
| data: { | ||
| tokenNames: "'--wpds-nonexistent'", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: `const token = 'var(--wpds-nonexistent-token)';`, | ||
| errors: [ | ||
| { | ||
| messageId: 'onlyKnownTokens', | ||
| data: { | ||
| tokenNames: "'--wpds-nonexistent-token'", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: 'const token = `var(--wpds-nonexistent-token)`;', | ||
| errors: [ | ||
| { | ||
| messageId: 'onlyKnownTokens', | ||
| data: { | ||
| tokenNames: "'--wpds-nonexistent-token'", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: 'const token = `var(--wpds-dimension-gap-${ size })`;', | ||
| errors: [ | ||
| { | ||
| messageId: 'dynamicToken', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: '<div style={ { gap: `var(--wpds-dimension-gap-${ size })` } } />', | ||
| errors: [ | ||
|
Comment on lines
+99
to
+109
|
||
| { | ||
| messageId: 'dynamicToken', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: `const token = '--wpds-nonexistent-token';`, | ||
| errors: [ | ||
| { | ||
| messageId: 'onlyKnownTokens', | ||
| data: { | ||
| tokenNames: "'--wpds-nonexistent-token'", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: 'const style = `--wpds-dimension-gap-${ size }`;', | ||
| errors: [ | ||
| { | ||
| messageId: 'dynamicToken', | ||
| }, | ||
| ], | ||
| }, | ||
|
Comment on lines
+78
to
+133
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| { | ||
| code: '`${ prefix }: var(--wpds-nonexistent-token)`', | ||
| errors: [ | ||
| { | ||
| messageId: 'onlyKnownTokens', | ||
| data: { | ||
| tokenNames: "'--wpds-nonexistent-token'", | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ function extractCSSVariables( value, prefix = '' ) { | |
| } | ||
|
|
||
| const knownTokens = new Set( tokenList ); | ||
| const wpdsTokensRegex = new RegExp( `[^\\w]--${ DS_TOKEN_PREFIX }`, 'i' ); | ||
| const wpdsTokensRegex = new RegExp( `(?:^|[^\\w])--${ DS_TOKEN_PREFIX }`, 'i' ); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tweaked to match the "start of string" case raised in #75872 (review).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests added in 8f0605b |
||
|
|
||
| module.exports = /** @type {import('eslint').Rule.RuleModule} */ ( { | ||
| meta: { | ||
|
|
@@ -48,27 +48,96 @@ module.exports = /** @type {import('eslint').Rule.RuleModule} */ ( { | |
| messages: { | ||
| onlyKnownTokens: | ||
| 'The following CSS variables are not valid Design System tokens: {{ tokenNames }}', | ||
| dynamicToken: | ||
| 'Design System tokens must not be dynamically constructed, as they cannot be statically verified for correctness or processed automatically to inject fallbacks.', | ||
| }, | ||
| }, | ||
| create( context ) { | ||
| const disallowedTokensAST = `JSXAttribute[name.name="style"] :matches(Literal[value=${ wpdsTokensRegex }], TemplateLiteral TemplateElement[value.raw=${ wpdsTokensRegex }])`; | ||
| const dynamicTemplateLiteralAST = `TemplateLiteral[expressions.length>0]:has(TemplateElement[value.raw=${ wpdsTokensRegex }])`; | ||
| const staticTokensAST = `:matches(Literal[value=${ wpdsTokensRegex }], TemplateLiteral[expressions.length=0] TemplateElement[value.raw=${ wpdsTokensRegex }])`; | ||
| const dynamicTokenEndRegex = new RegExp( | ||
| `--${ DS_TOKEN_PREFIX }[\\w-]*$` | ||
| ); | ||
|
|
||
| return { | ||
| /** | ||
| * For template literals with expressions, check each quasi | ||
| * individually: flag as dynamic only when a `--wpds-*` token | ||
| * name is split across a quasi/expression boundary, and | ||
| * validate any complete static tokens normally. | ||
| * | ||
| * @param {import('estree').TemplateLiteral} node | ||
| */ | ||
| [ dynamicTemplateLiteralAST ]( node ) { | ||
| let hasDynamic = false; | ||
| const unknownTokens = []; | ||
|
|
||
| for ( const quasi of node.quasis ) { | ||
| const raw = quasi.value.raw; | ||
| const value = quasi.value.cooked ?? raw; | ||
| const isFollowedByExpression = ! quasi.tail; | ||
|
|
||
| if ( | ||
| isFollowedByExpression && | ||
| dynamicTokenEndRegex.test( raw ) | ||
| ) { | ||
| hasDynamic = true; | ||
| } | ||
|
|
||
| const tokens = extractCSSVariables( | ||
| value, | ||
| DS_TOKEN_PREFIX | ||
| ); | ||
|
|
||
| // Remove the trailing incomplete token — it's the one | ||
| // being dynamically constructed by the next expression. | ||
| if ( isFollowedByExpression ) { | ||
| const endMatch = value.match( /(--([\w-]+))$/ ); | ||
| if ( endMatch ) { | ||
| tokens.delete( endMatch[ 1 ] ); | ||
| } | ||
| } | ||
|
|
||
| for ( const token of tokens ) { | ||
| if ( ! knownTokens.has( token ) ) { | ||
| unknownTokens.push( token ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if ( hasDynamic ) { | ||
| context.report( { | ||
| node, | ||
| messageId: 'dynamicToken', | ||
| } ); | ||
| } | ||
|
|
||
| if ( unknownTokens.length > 0 ) { | ||
| context.report( { | ||
| node, | ||
| messageId: 'onlyKnownTokens', | ||
| data: { | ||
| tokenNames: unknownTokens | ||
| .map( ( token ) => `'${ token }'` ) | ||
| .join( ', ' ), | ||
| }, | ||
| } ); | ||
| } | ||
| }, | ||
| /** @param {import('estree').Literal | import('estree').TemplateElement} node */ | ||
| [ disallowedTokensAST ]( node ) { | ||
| [ staticTokensAST ]( node ) { | ||
| let computedValue; | ||
|
|
||
| if ( ! node.value ) { | ||
| return; | ||
| } | ||
|
|
||
| if ( typeof node.value === 'string' ) { | ||
| // Get the node's value when it's a "string" | ||
| computedValue = node.value; | ||
| } else if ( | ||
| typeof node.value === 'object' && | ||
| 'raw' in node.value | ||
| ) { | ||
| // Get the node's value when it's a `template literal` | ||
| computedValue = node.value.cooked ?? node.value.raw; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.