From 76fce92b96d4e7769ed9219ff49ab6ae0bfe3715 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Wed, 30 Sep 2026 16:44:48 +0900 Subject: [PATCH] fix(eslint-plugin): check only the values the build reads as styles The array, typography and media rules reported and autofixed any value under a Devup UI element or call, including data in pass-through props (data-*, aria-*, handlers, HTML attributes, props, styleVars) and arguments of other functions. They now share one classifier mirroring the extractor, which also keeps the element context across nested elements. css-utils-literal-only reads a css() or keyframes() result held in a const of any scope as static. Refs #684. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../changepack_log_lint_style_positions.json | 7 + .../rules/css-utils-literal-only/README.md | 2 +- .../__tests__/index.test.ts | 24 + .../src/rules/css-utils-literal-only/index.ts | 15 +- .../src/rules/no-duplicate-value/README.md | 1 + .../__tests__/index.test.ts | 49 ++ .../src/rules/no-duplicate-value/index.ts | 30 +- .../no-typography-token-prefix/README.md | 4 +- .../__tests__/index.test.ts | 6 + .../rules/no-typography-token-prefix/index.ts | 23 +- .../src/rules/no-useless-responsive/README.md | 1 + .../__tests__/index.test.ts | 21 + .../src/rules/no-useless-responsive/index.ts | 41 +- .../rules/no-useless-tailing-nulls/README.md | 1 + .../__tests__/index.test.ts | 18 + .../rules/no-useless-tailing-nulls/index.ts | 32 +- .../rules/prefer-media-shorthand/README.md | 3 + .../__tests__/index.test.ts | 7 + .../src/rules/prefer-media-shorthand/index.ts | 32 +- .../utils/__tests__/style-position.test.ts | 46 ++ .../eslint-plugin/src/utils/style-position.ts | 508 ++++++++++++++++++ 21 files changed, 726 insertions(+), 145 deletions(-) create mode 100644 .changepacks/changepack_log_lint_style_positions.json create mode 100644 packages/eslint-plugin/src/utils/__tests__/style-position.test.ts create mode 100644 packages/eslint-plugin/src/utils/style-position.ts diff --git a/.changepacks/changepack_log_lint_style_positions.json b/.changepacks/changepack_log_lint_style_positions.json new file mode 100644 index 000000000..e26d12df9 --- /dev/null +++ b/.changepacks/changepack_log_lint_style_positions.json @@ -0,0 +1,7 @@ +{ + "changes": { + "packages/eslint-plugin/package.json": "Patch" + }, + "note": "no-duplicate-value, no-useless-responsive, no-useless-tailing-nulls, no-typography-token-prefix and prefer-media-shorthand only report and fix values the build reads as styles: style props of Box, Flex and the other style components and the arguments of css, globalCss and keyframes, through style objects, responsive arrays, conditions and spreads. Arrays and keys in props the component passes through (data-*, aria-*, event handlers, HTML attributes, props, styleVars), in arguments of other functions and under imports/fontFaces/params are left alone, where autofix used to rewrite them; styles of a component nested in another's prop are checked too. css-utils-literal-only reads a css() or keyframes() result held in a const of any scope as static, as the build does", + "date": "2026-09-30T00:00:00.000Z" +} diff --git a/packages/eslint-plugin/src/rules/css-utils-literal-only/README.md b/packages/eslint-plugin/src/rules/css-utils-literal-only/README.md index c10eed6d7..c9416e32c 100644 --- a/packages/eslint-plugin/src/rules/css-utils-literal-only/README.md +++ b/packages/eslint-plugin/src/rules/css-utils-literal-only/README.md @@ -13,7 +13,7 @@ The build knows: - literals, and constants: imports and module-level `const`s - what those compute through exact built-ins (`String`, `Number`, `JSON`, string and array methods, `Math.max`, `Math.round`, ... — not `Math.random`, `Math.sin` or `Math.pow`) and through functions this file declares that only compute - what StyleX functions give: `defineVars()` variables, `keyframes()` names, `firstThatWorks()` -- the class a devup-ui `css()` and the name a `keyframes()` give, held in a module-level `const` (`const fade = keyframes({ ... }); css({ animationName: fade })`), also through the package imported whole (`Devup.keyframes`) +- the class a devup-ui `css()` and the name a `keyframes()` give, held in a `const` of any scope (`const fade = keyframes({ ... }); css({ animationName: fade })`, also inside a component), also through the package imported whole (`Devup.keyframes`) The build inlines constants, folds `Math` and runs the file's own functions at build time. It never runs another module's code, so calling an imported function is reported. diff --git a/packages/eslint-plugin/src/rules/css-utils-literal-only/__tests__/index.test.ts b/packages/eslint-plugin/src/rules/css-utils-literal-only/__tests__/index.test.ts index 786e2f84a..fc7fe8169 100644 --- a/packages/eslint-plugin/src/rules/css-utils-literal-only/__tests__/index.test.ts +++ b/packages/eslint-plugin/src/rules/css-utils-literal-only/__tests__/index.test.ts @@ -135,12 +135,36 @@ describe.each(['css' /* 'globalCss', 'keyframes'*/])( code: `import { css, keyframes as kf, globalCss } from "@devup-ui/react";\nimport * as Devup from "@devup-ui/react";\nconst fade = kf({ from: { opacity: 0 } });\nconst spin = Devup.keyframes\`from { rotate: 0deg; }\`;\nconst base = css({ color: 'red' });\nconst tagged = css\`color: blue;\`;\ncss({ animationName: fade, animation: \`\${spin} 1s\`, selectors: { [\`.\${base} &\`]: { m: 1 } } });\nglobalCss({ body: { animationName: fade } });\nkf({ from: { opacity: 0 }, to: { content: \`"\${tagged}"\` } });`, filename: 'src/app/page.tsx', }, + { + code: `import { css, keyframes } from "@devup-ui/react";\nexport function C() { const fade = keyframes({ from: { opacity: 0 } }); const spin = keyframes\`from { rotate: 0deg; }\`; return css({ animationName: fade, animation: \`\${spin} 1s\` }); }`, + filename: 'src/app/page.tsx', + }, { code: `import * as stylex from "@stylexjs/stylex";\nimport sx, { create, defineVars as vars, props } from "@stylexjs/stylex";\nconst colors = stylex.defineVars({ c: 'red' });\nconst named = vars({ c: 'blue' });\nconst fade = stylex.keyframes({ from: { opacity: 0 } });\nconst styles = stylex.create({ a: { color: colors.c, animationName: fade, width: stylex.firstThatWorks('1px', 'auto') }, b: (w) => ({ width: w }) });\ncreate({ a: { color: named.c } });\nsx.create({ a: { color: 'red' } });\nstylex.createTheme(colors, { c: 'green' });\nstylex.props(styles.a, on);\nprops(on);`, filename: 'src/app/page.tsx', }, ], invalid: [ + ...[ + [ + `export function C() { let fade = keyframes({ from: { opacity: 0 } }); return css({ animationName: fade }); }`, + 1, + ], + [ + `export function C(v) { const fade = keyframes({ from: { opacity: v } }); return css({ animationName: fade }); }`, + 2, + ], + [ + `export function C() { const fade = other({ from: { opacity: 0 } }); return css({ animationName: fade }); }`, + 1, + ], + ].map(([code, count]) => ({ + code: `import { css, keyframes } from "@devup-ui/react";\n${code}`, + filename: 'src/app/page.tsx', + errors: Array.from({ length: Number(count) }, () => ({ + messageId: 'cssUtilsLiteralOnly' as const, + })), + })), ...[ [`let fade = keyframes({ from: { opacity: 0 } });`, 1], [`const fade = keyframes({ from: { opacity: v } });`, 2], diff --git a/packages/eslint-plugin/src/rules/css-utils-literal-only/index.ts b/packages/eslint-plugin/src/rules/css-utils-literal-only/index.ts index 3bc5ef3cb..c7f703ca0 100644 --- a/packages/eslint-plugin/src/rules/css-utils-literal-only/index.ts +++ b/packages/eslint-plugin/src/rules/css-utils-literal-only/index.ts @@ -628,6 +628,14 @@ class Values { private readonly givesStyleName: (callee: TSESTree.Node) => boolean, ) {} + /** Whether `init` is a call of `css()` or `keyframes()`, whose result the build writes in place of a `const` holding it in any scope */ + private holdsStyleName(init: TSESTree.Expression): boolean { + return init.type === AST_NODE_TYPES.TaggedTemplateExpression + ? this.givesStyleName(init.tag) + : init.type === AST_NODE_TYPES.CallExpression && + this.givesStyleName(init.callee) + } + /** Whether reading member `name` of `object` gives the same on every engine and page */ private exactMember( object: TSESTree.Node, @@ -766,9 +774,12 @@ class Values { if ( definition.type !== 'Variable' || definition.parent.kind !== 'const' || - !['module', 'global'].includes(variable.scope.type) || definition.node.id.type !== AST_NODE_TYPES.Identifier || - !definition.node.init + !definition.node.init || + !( + ['module', 'global'].includes(variable.scope.type) || + this.holdsStyleName(definition.node.init) + ) ) return false seen.add(name) diff --git a/packages/eslint-plugin/src/rules/no-duplicate-value/README.md b/packages/eslint-plugin/src/rules/no-duplicate-value/README.md index b5774d5b9..bb2c4e04b 100644 --- a/packages/eslint-plugin/src/rules/no-duplicate-value/README.md +++ b/packages/eslint-plugin/src/rules/no-duplicate-value/README.md @@ -65,6 +65,7 @@ The rule will not trigger for: - Arrays used with other libraries - Non-literal values - Arrays that are part of member expressions +- Arrays the build does not read as styles: in props the component passes through (`data-*`, `aria-*`, event handlers, HTML attributes, `props`, `styleVars`), in arguments of other functions, and under `imports`/`fontFaces`/`params` ## Auto-fixable diff --git a/packages/eslint-plugin/src/rules/no-duplicate-value/__tests__/index.test.ts b/packages/eslint-plugin/src/rules/no-duplicate-value/__tests__/index.test.ts index 2c529a94a..9d41d54dd 100644 --- a/packages/eslint-plugin/src/rules/no-duplicate-value/__tests__/index.test.ts +++ b/packages/eslint-plugin/src/rules/no-duplicate-value/__tests__/index.test.ts @@ -44,6 +44,24 @@ describe('no-duplicate-value rule', () => { code: 'import { Box } from "@devup-ui/react";\n', filename: 'src/app/page.tsx', }, + ...[ + '', + '', + ' pick([5, 5])} />', + '', + '', + '', + '', + '', + '', + 'getTheme([5, 5])', + 'Devup.getTheme([5, 5])', + 'globalCss({ imports: ["a.css", "a.css"] })', + 'css({ [[5, 5]]: 1 })', + ].map((use) => ({ + code: `import { Box, ThemeScript, getTheme, globalCss, css } from "@devup-ui/react";\nimport * as Devup from "@devup-ui/react";\n${use}`, + filename: 'src/app/page.tsx', + })), ], invalid: [ { @@ -99,6 +117,37 @@ describe('no-duplicate-value rule', () => { }, ], }, + ...[ + ['', ''], + ['', ''], + ['', ''], + ['', ''], + [ + '', + '', + ], + ['', ''], + ['', ''], + ['', ''], + ['Devup.css({ w: [1, 1] })', 'Devup.css({ w: [1, null] })'], + ['css(base, { w: [1, 1] })', 'css(base, { w: [1, null] })'], + ['css({ ...{ w: [1, 1] } })', 'css({ ...{ w: [1, null] } })'], + ].map(([use, fixed]) => ({ + code: `import { Box, css } from "@devup-ui/react";\nimport * as Devup from "@devup-ui/react";\n${use}`, + output: `import { Box, css } from "@devup-ui/react";\nimport * as Devup from "@devup-ui/react";\n${fixed}`, + filename: 'src/app/page.tsx', + errors: [{ messageId: 'duplicateValue' as const }], + })), + { + code: 'import { Box } from "@devup-ui/react";\n} m={[2, 2]} />', + output: + 'import { Box } from "@devup-ui/react";\n} m={[2, null]} />', + filename: 'src/app/page.tsx', + errors: [ + { messageId: 'duplicateValue' }, + { messageId: 'duplicateValue' }, + ], + }, ], }) }) diff --git a/packages/eslint-plugin/src/rules/no-duplicate-value/index.ts b/packages/eslint-plugin/src/rules/no-duplicate-value/index.ts index c5e34621d..742218679 100644 --- a/packages/eslint-plugin/src/rules/no-duplicate-value/index.ts +++ b/packages/eslint-plugin/src/rules/no-duplicate-value/index.ts @@ -6,6 +6,7 @@ import { import type { RuleContext } from '@typescript-eslint/utils/ts-eslint' import { ImportStorage } from '../../utils/import-storage' +import { styleValueRoot } from '../../utils/style-position' const createRule = ESLintUtils.RuleCreator( (name) => @@ -70,38 +71,13 @@ export const noDuplicateValue = createRule({ }, create(context) { const importStorage = new ImportStorage() - let devupContext: - TSESTree.CallExpression | TSESTree.JSXOpeningElement | null = null return { ImportDeclaration(node) { importStorage.addImportByDeclaration(node) }, - CallExpression(node) { - if ( - importStorage.checkContextType(node) === 'UTIL' && - node.arguments.length === 1 && - node.arguments[0].type === AST_NODE_TYPES.ObjectExpression - ) { - devupContext = node - } - }, - 'CallExpression:exit'(node) { - if (devupContext === node) { - devupContext = null - } - }, - JSXOpeningElement(node) { - if (importStorage.checkContextType(node) === 'COMPONENT') { - devupContext = node - } - }, - 'JSXOpeningElement:exit'(node) { - if (devupContext === node) { - devupContext = null - } - }, ArrayExpression(node) { - if (devupContext) checkDuplicateValue(node, context) + if (styleValueRoot(node, importStorage)) + checkDuplicateValue(node, context) }, } }, diff --git a/packages/eslint-plugin/src/rules/no-typography-token-prefix/README.md b/packages/eslint-plugin/src/rules/no-typography-token-prefix/README.md index a8d91d3a9..66e714128 100644 --- a/packages/eslint-plugin/src/rules/no-typography-token-prefix/README.md +++ b/packages/eslint-plugin/src/rules/no-typography-token-prefix/README.md @@ -10,7 +10,9 @@ preset that does not exist, so no style is applied. The rule checks string values of `typography` on Devup UI components and utilities, including values inside responsive arrays, conditionals, and -selector objects. +selector objects. A `typography` key the build does not read as a style — in a +prop the component passes through (`data-*`, `props`, ...) or in an argument of +another function — is not checked. ### Examples diff --git a/packages/eslint-plugin/src/rules/no-typography-token-prefix/__tests__/index.test.ts b/packages/eslint-plugin/src/rules/no-typography-token-prefix/__tests__/index.test.ts index 9c0395101..aa2c8e316 100644 --- a/packages/eslint-plugin/src/rules/no-typography-token-prefix/__tests__/index.test.ts +++ b/packages/eslint-plugin/src/rules/no-typography-token-prefix/__tests__/index.test.ts @@ -24,6 +24,12 @@ describe('no-typography-token-prefix rule', () => { { code: `${imports}css({ typography: 1 })` }, { code: `import { Box } from "other";\n` }, { code: `const a = { typography: '$heading' }` }, + { code: `${imports}` }, + { code: `${imports}` }, + { code: `${imports}css({ w: pick({ typography: '$heading' }) })` }, + { + code: `import { setTheme } from "@devup-ui/react";\nsetTheme({ typography: '$heading' })`, + }, ], invalid: [ { diff --git a/packages/eslint-plugin/src/rules/no-typography-token-prefix/index.ts b/packages/eslint-plugin/src/rules/no-typography-token-prefix/index.ts index 31ae5ec2a..45ab1276c 100644 --- a/packages/eslint-plugin/src/rules/no-typography-token-prefix/index.ts +++ b/packages/eslint-plugin/src/rules/no-typography-token-prefix/index.ts @@ -6,6 +6,7 @@ import { import { ImportStorage } from '../../utils/import-storage' import { propertyKeyName } from '../../utils/property-key-name' +import { styleValueRoot } from '../../utils/style-position' const createRule = ESLintUtils.RuleCreator( (name) => @@ -53,34 +54,16 @@ export const noTypographyTokenPrefix = createRule({ }, create(context) { const importStorage = new ImportStorage() - let devupContext: - TSESTree.CallExpression | TSESTree.JSXOpeningElement | null = null return { ImportDeclaration(node) { importStorage.addImportByDeclaration(node) }, - CallExpression(node) { - if (!devupContext && importStorage.checkContextType(node) === 'UTIL') { - devupContext = node - } - }, - 'CallExpression:exit'(node) { - if (devupContext === node) devupContext = null - }, - JSXOpeningElement(node) { - if (importStorage.checkContextType(node) === 'COMPONENT') { - devupContext = node - } - }, - 'JSXOpeningElement:exit'(node) { - if (devupContext === node) devupContext = null - }, Literal(node) { if ( - !devupContext || typeof node.value !== 'string' || !node.value.startsWith('$') || - !isTypographyValue(node) + !isTypographyValue(node) || + !styleValueRoot(node, importStorage) ) return const name = node.value.slice(1) diff --git a/packages/eslint-plugin/src/rules/no-useless-responsive/README.md b/packages/eslint-plugin/src/rules/no-useless-responsive/README.md index 481c4a365..9dc3ad940 100644 --- a/packages/eslint-plugin/src/rules/no-useless-responsive/README.md +++ b/packages/eslint-plugin/src/rules/no-useless-responsive/README.md @@ -73,6 +73,7 @@ The rule will not trigger for: - Empty arrays (e.g., `[]`) - Arrays used with other libraries - Non-array values +- Arrays the build does not read as styles: in props the component passes through (`data-*`, `aria-*`, event handlers, HTML attributes, `props`, `styleVars`), in arguments of other functions, and under `imports`/`fontFaces`/`params` ## Auto-fixable diff --git a/packages/eslint-plugin/src/rules/no-useless-responsive/__tests__/index.test.ts b/packages/eslint-plugin/src/rules/no-useless-responsive/__tests__/index.test.ts index ca558911a..ccdfba6af 100644 --- a/packages/eslint-plugin/src/rules/no-useless-responsive/__tests__/index.test.ts +++ b/packages/eslint-plugin/src/rules/no-useless-responsive/__tests__/index.test.ts @@ -89,8 +89,29 @@ describe('no-useless-responsive rule', () => { code: 'import { globalCss } from "@devup-ui/react";\nglobalCss({ imports: [{"url": "@devup-ui/react/css/global.css"}] })', filename: 'src/app/page.tsx', }, + ...[ + '', + '', + '', + '', + 'css({ w: pick([1]) })', + 'globalCss({ fontFaces: [{ fontFamily: "a" }] })', + ].map((use) => ({ + code: `import { Box, ThemeScript, css, globalCss } from "@devup-ui/react";\n${use}`, + filename: 'src/app/page.tsx', + })), ], invalid: [ + { + code: 'import { Box } from "@devup-ui/react";\n} m={[2]} />', + output: + 'import { Box } from "@devup-ui/react";\n} m={2} />', + filename: 'src/app/page.tsx', + errors: [ + { messageId: 'uselessResponsive' }, + { messageId: 'uselessResponsive' }, + ], + }, { code: 'import { Box } from "@devup-ui/react";\n', output: 'import { Box } from "@devup-ui/react";\n', diff --git a/packages/eslint-plugin/src/rules/no-useless-responsive/index.ts b/packages/eslint-plugin/src/rules/no-useless-responsive/index.ts index 5de298a2e..73fed13cf 100644 --- a/packages/eslint-plugin/src/rules/no-useless-responsive/index.ts +++ b/packages/eslint-plugin/src/rules/no-useless-responsive/index.ts @@ -6,6 +6,7 @@ import { import type { RuleContext } from '@typescript-eslint/utils/ts-eslint' import { ImportStorage } from '../../utils/import-storage' +import { styleValueRoot } from '../../utils/style-position' const createRule = ESLintUtils.RuleCreator( (name) => @@ -63,52 +64,18 @@ export const noUselessResponsive = createRule({ }, create(context) { const importStorage = new ImportStorage() - let devupContext: - TSESTree.CallExpression | TSESTree.JSXOpeningElement | null = null return { ImportDeclaration(node) { importStorage.addImportByDeclaration(node) }, - CallExpression(node) { - if ( - importStorage.checkContextType(node) === 'UTIL' && - node.arguments.length === 1 && - node.arguments[0].type === AST_NODE_TYPES.ObjectExpression - ) { - devupContext = node - } - }, - 'CallExpression:exit'(node) { - if (devupContext === node) { - devupContext = null - } - }, - JSXOpeningElement(node) { - if (importStorage.checkContextType(node) === 'COMPONENT') { - devupContext = node - } - }, - 'JSXOpeningElement:exit'(node) { - if (devupContext === node) { - devupContext = null - } - }, - Property(node) { - if ( - devupContext && - node.key.type === AST_NODE_TYPES.Identifier && - ['imports', 'params', 'fontFaces'].includes(node.key.name) - ) { - devupContext = null - } - }, ArrayExpression(node) { - if (devupContext) + const root = styleValueRoot(node, importStorage) + if (root) checkUselessResponsive( node, context.sourceCode .getAncestors(node) - .slice(context.sourceCode.getAncestors(devupContext).length), + .slice(context.sourceCode.getAncestors(root).length), context, ) }, diff --git a/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/README.md b/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/README.md index 11a95db0e..ed87222f2 100644 --- a/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/README.md +++ b/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/README.md @@ -57,6 +57,7 @@ The rule will not trigger for: - Arrays with null values in the middle - Arrays used with other libraries - Arrays that are part of member expressions +- Arrays the build does not read as styles: in props the component passes through (`data-*`, `aria-*`, event handlers, HTML attributes, `props`, `styleVars`), in arguments of other functions, and under `imports`/`fontFaces`/`params` ## Auto-fixable diff --git a/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/__tests__/index.test.ts b/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/__tests__/index.test.ts index 4a67a3c72..8af51b383 100644 --- a/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/__tests__/index.test.ts +++ b/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/__tests__/index.test.ts @@ -34,8 +34,26 @@ describe('no-useless-tailing-nulls rule', () => { code: 'css({ w: [1, 2, null] })', filename: 'src/app/page.tsx', }, + ...[ + '', + '', + '', + ].map((use) => ({ + code: `import { Box } from "@devup-ui/react";\n${use}`, + filename: 'src/app/page.tsx', + })), ], invalid: [ + { + code: 'import { Box } from "@devup-ui/react";\n} m={[2, null]} />', + output: + 'import { Box } from "@devup-ui/react";\n} m={[2]} />', + filename: 'src/app/page.tsx', + errors: [ + { messageId: 'uselessTailingNulls' }, + { messageId: 'uselessTailingNulls' }, + ], + }, { code: 'import { Box } from "@devup-ui/react";\n', output: 'import { Box } from "@devup-ui/react";\n', diff --git a/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/index.ts b/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/index.ts index de6b9a0db..c87447748 100644 --- a/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/index.ts +++ b/packages/eslint-plugin/src/rules/no-useless-tailing-nulls/index.ts @@ -6,6 +6,7 @@ import { import type { RuleContext } from '@typescript-eslint/utils/ts-eslint' import { ImportStorage } from '../../utils/import-storage' +import { styleValueRoot } from '../../utils/style-position' const createRule = ESLintUtils.RuleCreator( (name) => @@ -62,41 +63,12 @@ export const noUselessTailingNulls = createRule({ }, create(context) { const importStorage = new ImportStorage() - let devupContext: - TSESTree.CallExpression | TSESTree.JSXOpeningElement | null = null return { ImportDeclaration(node) { importStorage.addImportByDeclaration(node) }, - CallExpression(node) { - if ( - importStorage.checkContextType(node) === 'UTIL' && - node.arguments.length === 1 && - node.arguments[0].type === AST_NODE_TYPES.ObjectExpression - ) { - devupContext = node - } - }, - 'CallExpression:exit'(node) { - if (devupContext === node) { - devupContext = null - } - }, - JSXOpeningElement(node) { - if (importStorage.checkContextType(node) === 'COMPONENT') { - devupContext = node - } - }, - 'JSXOpeningElement:exit'(node) { - if (devupContext === node) { - devupContext = null - } - }, ArrayExpression(node) { - if ( - devupContext && - node.parent?.type !== AST_NODE_TYPES.MemberExpression - ) + if (styleValueRoot(node, importStorage)) checkUselessTailingNulls(node, context) }, } diff --git a/packages/eslint-plugin/src/rules/prefer-media-shorthand/README.md b/packages/eslint-plugin/src/rules/prefer-media-shorthand/README.md index 93128c120..f19a28ce6 100644 --- a/packages/eslint-plugin/src/rules/prefer-media-shorthand/README.md +++ b/packages/eslint-plugin/src/rules/prefer-media-shorthand/README.md @@ -6,6 +6,9 @@ Prefer the media shorthand props over spelling out the same media query. `_media` entries and `'@media …'` keys whose query is exactly one of the shorthands are reported. Whitespace and letter case in the query are ignored. +Keys the build does not read as styles — in a prop the component passes through +(`data-*`, `props`, ...) or in an argument of another function — are not +checked. | Query | Shorthand | | ---------------------------------------- | ---------------- | diff --git a/packages/eslint-plugin/src/rules/prefer-media-shorthand/__tests__/index.test.ts b/packages/eslint-plugin/src/rules/prefer-media-shorthand/__tests__/index.test.ts index df0232e17..8afb1caf6 100644 --- a/packages/eslint-plugin/src/rules/prefer-media-shorthand/__tests__/index.test.ts +++ b/packages/eslint-plugin/src/rules/prefer-media-shorthand/__tests__/index.test.ts @@ -29,6 +29,13 @@ describe('prefer-media-shorthand rule', () => { code: `import { Box } from "other";\n`, }, { code: `css({ '@media print': { color: 'red' } })` }, + { + code: `${imports}`, + }, + { code: `${imports}` }, + { + code: `${imports}`, + }, ], invalid: [ { diff --git a/packages/eslint-plugin/src/rules/prefer-media-shorthand/index.ts b/packages/eslint-plugin/src/rules/prefer-media-shorthand/index.ts index c82d25d4a..5a207664c 100644 --- a/packages/eslint-plugin/src/rules/prefer-media-shorthand/index.ts +++ b/packages/eslint-plugin/src/rules/prefer-media-shorthand/index.ts @@ -6,6 +6,7 @@ import { import { ImportStorage } from '../../utils/import-storage' import { propertyKeyName } from '../../utils/property-key-name' +import { styleValueRoot } from '../../utils/style-position' const createRule = ESLintUtils.RuleCreator( (name) => @@ -47,8 +48,6 @@ export const preferMediaShorthand = createRule({ }, create(context) { const importStorage = new ImportStorage() - let devupContext: - TSESTree.CallExpression | TSESTree.JSXOpeningElement | null = null function checkMediaRecord( owner: TSESTree.Property | TSESTree.JSXAttribute, @@ -83,41 +82,20 @@ export const preferMediaShorthand = createRule({ ImportDeclaration(node) { importStorage.addImportByDeclaration(node) }, - CallExpression(node) { - if ( - !devupContext && - importStorage.checkContextType(node) === 'UTIL' && - node.arguments.length === 1 && - node.arguments[0].type === AST_NODE_TYPES.ObjectExpression - ) { - devupContext = node - } - }, - 'CallExpression:exit'(node) { - if (devupContext === node) devupContext = null - }, - JSXOpeningElement(node) { - if (importStorage.checkContextType(node) === 'COMPONENT') { - devupContext = node - } - }, - 'JSXOpeningElement:exit'(node) { - if (devupContext === node) devupContext = null - }, JSXAttribute(node) { if ( - devupContext && node.name.type === AST_NODE_TYPES.JSXIdentifier && node.name.name === '_media' && - node.value?.type === AST_NODE_TYPES.JSXExpressionContainer + node.value?.type === AST_NODE_TYPES.JSXExpressionContainer && + styleValueRoot(node.value.expression, importStorage) ) { checkMediaRecord(node, node.value.expression) } }, Property(node) { - if (!devupContext) return const name = propertyKeyName(node) - if (name === undefined) return + if (name === undefined || !styleValueRoot(node.value, importStorage)) + return if (name === '_media') { checkMediaRecord(node, node.value) return diff --git a/packages/eslint-plugin/src/utils/__tests__/style-position.test.ts b/packages/eslint-plugin/src/utils/__tests__/style-position.test.ts new file mode 100644 index 000000000..d05187d95 --- /dev/null +++ b/packages/eslint-plugin/src/utils/__tests__/style-position.test.ts @@ -0,0 +1,46 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' + +import { describe, expect, it } from 'bun:test' + +import { isPassThroughProp, SPECIAL_PROPERTIES } from '../style-position' + +describe('style-position', () => { + it('passes through exactly the props the build does', () => { + const rust = readFileSync( + join( + import.meta.dir, + '../../../../../libs/css/src/is_special_property.rs', + ), + 'utf8', + ) + const start = rust.indexOf('static SPECIAL_PROPERTIES') + const names = [ + ...rust.slice(start, rust.indexOf('};', start)).matchAll(/"([^"]+)"/g), + ].map((match) => match[1]) + expect([...SPECIAL_PROPERTIES].sort()).toEqual([...new Set(names)].sort()) + }) + + it('reads every other prop as a style', () => { + for (const name of [ + 'onClick', + 'data-id', + 'aria-label', + 'className', + 'as', + 'props', + 'styleVars', + 'styleOrder', + ]) + expect(isPassThroughProp(name)).toBe(true) + for (const name of [ + 'bg', + 'w', + 'items', + '_hover', + 'selectors', + 'typography', + ]) + expect(isPassThroughProp(name)).toBe(false) + }) +}) diff --git a/packages/eslint-plugin/src/utils/style-position.ts b/packages/eslint-plugin/src/utils/style-position.ts new file mode 100644 index 000000000..9a25c2666 --- /dev/null +++ b/packages/eslint-plugin/src/utils/style-position.ts @@ -0,0 +1,508 @@ +import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils' + +import type { ImportStorage } from './import-storage' + +/** The HTML, SVG and React attributes a Devup UI component passes through instead of reading as styles, as `is_special_property` in `libs/css/src/is_special_property.rs` lists them */ +export const SPECIAL_PROPERTIES = new Set([ + 'dangerouslySetInnerHTML', + 'children', + 'key', + 'ref', + 'defaultChecked', + 'defaultValue', + 'suppressContentEditableWarning', + 'suppressHydrationWarning', + 'accessKey', + 'autoCapitalize', + 'autoFocus', + 'className', + 'contentEditable', + 'contextMenu', + 'dir', + 'draggable', + 'enterKeyHint', + 'hidden', + 'id', + 'lang', + 'nonce', + 'slot', + 'spellCheck', + 'style', + 'tabIndex', + 'title', + 'radioGroup', + 'role', + 'about', + 'datatype', + 'inlist', + 'prefix', + 'property', + 'rel', + 'resource', + 'rev', + 'typeof', + 'vocab', + 'autoCorrect', + 'autoSave', + 'itemProp', + 'itemScope', + 'itemType', + 'itemID', + 'itemRef', + 'results', + 'security', + 'unselectable', + 'popover', + 'popoverTargetAction', + 'popoverTarget', + 'inert', + 'inputMode', + 'is', + 'exportparts', + 'part', + 'accept', + 'acceptCharset', + 'action', + 'allowFullScreen', + 'allowTransparency', + 'alt', + 'async', + 'autoComplete', + 'autoPlay', + 'capture', + 'cellPadding', + 'cellSpacing', + 'charSet', + 'challenge', + 'checked', + 'cite', + 'classID', + 'cols', + 'colSpan', + 'controls', + 'coords', + 'crossOrigin', + 'data', + 'dateTime', + 'default', + 'defer', + 'disabled', + 'download', + 'encType', + 'form', + 'formAction', + 'formEncType', + 'formMethod', + 'formNoValidate', + 'formTarget', + 'frameBorder', + 'headers', + 'high', + 'href', + 'hrefLang', + 'htmlFor', + 'httpEquiv', + 'integrity', + 'keyParams', + 'keyType', + 'kind', + 'label', + 'list', + 'loop', + 'low', + 'manifest', + 'marginHeight', + 'marginWidth', + 'max', + 'maxLength', + 'media', + 'mediaGroup', + 'method', + 'min', + 'minLength', + 'multiple', + 'muted', + 'name', + 'noValidate', + 'open', + 'optimum', + 'pattern', + 'placeholder', + 'playsInline', + 'poster', + 'preload', + 'readOnly', + 'required', + 'reversed', + 'rows', + 'rowSpan', + 'sandbox', + 'scope', + 'scoped', + 'scrolling', + 'seamless', + 'selected', + 'shape', + 'size', + 'sizes', + 'span', + 'src', + 'srcDoc', + 'srcLang', + 'srcSet', + 'start', + 'step', + 'summary', + 'target', + 'type', + 'useMap', + 'value', + 'wmode', + 'wrap', + 'ping', + 'referrerPolicy', + 'allow', + 'loading', + 'decoding', + 'fetchPriority', + 'blocking', + 'imageSrcSet', + 'imageSizes', + 'precedence', + 'controlsList', + 'noModule', + 'align', + 'bgcolor', + 'frame', + 'rules', + 'dirName', + 'abbr', + 'valign', + 'disablePictureInPicture', + 'disableRemotePlayback', + 'accentHeight', + 'accumulate', + 'additive', + 'allowReorder', + 'alphabetic', + 'amplitude', + 'arabicForm', + 'ascent', + 'attributeName', + 'attributeType', + 'autoReverse', + 'azimuth', + 'baseFrequency', + 'baseProfile', + 'bbox', + 'begin', + 'bias', + 'by', + 'calcMode', + 'capHeight', + 'clipPathUnits', + 'colorProfile', + 'colorRendering', + 'contentScriptType', + 'contentStyleType', + 'decelerate', + 'descent', + 'diffuseConstant', + 'divisor', + 'dur', + 'dx', + 'dy', + 'edgeMode', + 'elevation', + 'enableBackground', + 'end', + 'exponent', + 'externalResourcesRequired', + 'filterRes', + 'filterUnits', + 'focusable', + 'format', + 'fr', + 'from', + 'fx', + 'fy', + 'g1', + 'g2', + 'glyphName', + 'glyphOrientationHorizontal', + 'glyphOrientationVertical', + 'glyphRef', + 'gradientTransform', + 'gradientUnits', + 'hanging', + 'horizAdvX', + 'horizOriginX', + 'ideographic', + 'in2', + 'in', + 'intercept', + 'k1', + 'k2', + 'k3', + 'k4', + 'k', + 'kernelMatrix', + 'kernelUnitLength', + 'kerning', + 'keyPoints', + 'keySplines', + 'keyTimes', + 'lengthAdjust', + 'limitingConeAngle', + 'local', + 'markerHeight', + 'markerUnits', + 'markerWidth', + 'maskContentUnits', + 'maskUnits', + 'mathematical', + 'mode', + 'numOctaves', + 'operator', + 'orient', + 'orientation', + 'origin', + 'overlinePosition', + 'overlineThickness', + 'panose1', + 'path', + 'pathLength', + 'patternContentUnits', + 'patternTransform', + 'patternUnits', + 'points', + 'pointsAtX', + 'pointsAtY', + 'pointsAtZ', + 'preserveAlpha', + 'preserveAspectRatio', + 'primitiveUnits', + 'radius', + 'refX', + 'refY', + 'renderingIntent', + 'repeatCount', + 'repeatDur', + 'requiredExtensions', + 'requiredFeatures', + 'restart', + 'result', + 'seed', + 'slope', + 'spacing', + 'specularConstant', + 'specularExponent', + 'speed', + 'spreadMethod', + 'startOffset', + 'stdDeviation', + 'stemh', + 'stemv', + 'stitchTiles', + 'strikethroughPosition', + 'strikethroughThickness', + 'string', + 'surfaceScale', + 'systemLanguage', + 'tableValues', + 'targetX', + 'targetY', + 'textLength', + 'to', + 'u1', + 'u2', + 'underlinePosition', + 'underlineThickness', + 'unicode', + 'unicodeRange', + 'unitsPerEm', + 'vAlphabetic', + 'values', + 'version', + 'vertAdvY', + 'vertOriginX', + 'vertOriginY', + 'vHanging', + 'vIdeographic', + 'viewBox', + 'viewTarget', + 'vMathematical', + 'widths', + 'x1', + 'x2', + 'xChannelSelector', + 'xHeight', + 'xlinkActuate', + 'xlinkArcrole', + 'xlinkHref', + 'xlinkRole', + 'xlinkShow', + 'xlinkTitle', + 'xlinkType', + 'xmlBase', + 'xmlLang', + 'xmlns', + 'xmlnsXlink', + 'xmlSpace', + 'y1', + 'y2', + 'yChannelSelector', + 'z', + 'zoomAndPan', + 'allowpopups', + 'autosize', + 'blinkfeatures', + 'disableblinkfeatures', + 'disableguestresize', + 'disablewebsecurity', + 'guestinstance', + 'httpreferrer', + 'nodeintegration', + 'partition', + 'plugins', + 'useragent', + 'webpreferences', +]) + +/** Props a Devup UI component reads itself rather than as styles */ +const OWN_PROPS = new Set(['as', 'props', 'styleVars', 'styleOrder']) + +/** Keys of style objects holding data rather than style values */ +const DATA_KEYS = new Set(['imports', 'fontFaces', 'params']) + +const STYLE_FUNCTIONS = new Set(['css', 'globalCss', 'keyframes']) + +const STYLE_COMPONENTS = new Set([ + 'Box', + 'Button', + 'Center', + 'Flex', + 'Grid', + 'Image', + 'Input', + 'Text', + 'VStack', +]) + +/** Whether a Devup UI component passes the prop `name` through instead of reading it as a style */ +export function isPassThroughProp(name: string): boolean { + return ( + name.startsWith('on') || + name.startsWith('data-') || + name.startsWith('aria-') || + SPECIAL_PROPERTIES.has(name) || + OWN_PROPS.has(name) + ) +} + +function isStyleComponent( + name: TSESTree.JSXTagNameExpression, + importStorage: ImportStorage, +): boolean { + if (name.type === AST_NODE_TYPES.JSXIdentifier) + return STYLE_COMPONENTS.has(importStorage.importedName(name.name) ?? '') + return ( + name.type === AST_NODE_TYPES.JSXMemberExpression && + name.object.type === AST_NODE_TYPES.JSXIdentifier && + importStorage.isImportObject(name.object.name) && + STYLE_COMPONENTS.has(name.property.name) + ) +} + +function isStyleFunction( + callee: TSESTree.Expression, + importStorage: ImportStorage, +): boolean { + if (callee.type === AST_NODE_TYPES.Identifier) + return STYLE_FUNCTIONS.has(importStorage.importedName(callee.name) ?? '') + return ( + callee.type === AST_NODE_TYPES.MemberExpression && + !callee.computed && + callee.object.type === AST_NODE_TYPES.Identifier && + callee.property.type === AST_NODE_TYPES.Identifier && + importStorage.isImportObject(callee.object.name) && + STYLE_FUNCTIONS.has(callee.property.name) + ) +} + +/** The Devup UI style component or style function closest above `node` */ +export function styleRoot( + node: TSESTree.Node, + importStorage: ImportStorage, +): TSESTree.JSXOpeningElement | TSESTree.CallExpression | null { + for (let current = node.parent; current; current = current.parent) { + if ( + current.type === AST_NODE_TYPES.JSXOpeningElement && + isStyleComponent(current.name, importStorage) + ) + return current + if ( + current.type === AST_NODE_TYPES.CallExpression && + isStyleFunction(current.callee, importStorage) + ) + return current + } + return null +} + +/** Whether the build reads what `parent` holds in `child` as a style value: a style prop, a style object value, a responsive array, a branch of a condition or a spread */ +function holdsStyle(parent: TSESTree.Node, child: TSESTree.Node): boolean { + switch (parent.type) { + case AST_NODE_TYPES.ObjectExpression: + case AST_NODE_TYPES.ArrayExpression: + case AST_NODE_TYPES.SpreadElement: + case AST_NODE_TYPES.LogicalExpression: + case AST_NODE_TYPES.JSXExpressionContainer: + case AST_NODE_TYPES.JSXSpreadAttribute: + case AST_NODE_TYPES.TSAsExpression: + case AST_NODE_TYPES.TSSatisfiesExpression: + case AST_NODE_TYPES.TSNonNullExpression: + return true + case AST_NODE_TYPES.Property: + return ( + parent.value === child && + !( + parent.key.type === AST_NODE_TYPES.Identifier && + !parent.computed && + DATA_KEYS.has(parent.key.name) + ) + ) + case AST_NODE_TYPES.ConditionalExpression: + return parent.test !== child + case AST_NODE_TYPES.JSXAttribute: + return ( + parent.name.type === AST_NODE_TYPES.JSXIdentifier && + !isPassThroughProp(parent.name.name) + ) + default: + return false + } +} + +/** Whether the build reads `node` as a style value of `root`, every node between them holding it as a style */ +export function isStylePosition( + node: TSESTree.Node, + root: TSESTree.Node, +): boolean { + let child = node + let parent = node.parent + while (parent && parent !== root && holdsStyle(parent, child)) { + child = parent + parent = parent.parent + } + return parent === root +} + +/** The Devup UI style component or function reading `node` as a style value, if one does */ +export function styleValueRoot( + node: TSESTree.Node, + importStorage: ImportStorage, +): TSESTree.JSXOpeningElement | TSESTree.CallExpression | null { + const root = styleRoot(node, importStorage) + return root && isStylePosition(node, root) ? root : null +}