From e729a723be10b63c566ce764738d4ba60f51e2e4 Mon Sep 17 00:00:00 2001 From: Ben Keen Date: Wed, 21 Aug 2024 01:31:01 -0700 Subject: [PATCH] feat(eslint-plugin): add top-level-styles eslint rule (#595) * Add top-level-styles eslint rule * Change files * code review feedback --- ...-dd20b1e1-c272-409a-95f9-19ca824187ce.json | 7 ++ packages/eslint-plugin/README.md | 5 +- .../eslint-plugin/src/rules/styles-file.ts | 34 +---- .../src/rules/top-level-styles.md | 62 ++++++++++ .../src/rules/top-level-styles.test.ts | 116 ++++++++++++++++++ .../src/rules/top-level-styles.ts | 51 ++++++++ packages/eslint-plugin/src/utils/helpers.ts | 62 ++++++++-- 7 files changed, 292 insertions(+), 45 deletions(-) create mode 100644 change/@griffel-eslint-plugin-dd20b1e1-c272-409a-95f9-19ca824187ce.json create mode 100644 packages/eslint-plugin/src/rules/top-level-styles.md create mode 100644 packages/eslint-plugin/src/rules/top-level-styles.test.ts create mode 100644 packages/eslint-plugin/src/rules/top-level-styles.ts diff --git a/change/@griffel-eslint-plugin-dd20b1e1-c272-409a-95f9-19ca824187ce.json b/change/@griffel-eslint-plugin-dd20b1e1-c272-409a-95f9-19ca824187ce.json new file mode 100644 index 000000000..480df200b --- /dev/null +++ b/change/@griffel-eslint-plugin-dd20b1e1-c272-409a-95f9-19ca824187ce.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "feat: add top-level-styles eslint rule", + "packageName": "@griffel/eslint-plugin", + "email": "ben.keen@gmail.com", + "dependentChangeType": "patch" +} diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 7bf5e0e2d..d3d111b48 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -23,7 +23,7 @@ Add `@griffel` to the plugins section and `plugin:@griffel/recommended` to the e ## Shareable configurations -This plugin exports recommended configuration that enforce good practices, but you can use only uses that are required for your use case: +This plugin exports recommended configuration that enforce good practices, but you can choose to use only the ones that are necessary for your use case: ```json { @@ -47,5 +47,6 @@ You can find more info about enabled rules in the [Supported Rules section](#sup | [`@griffel/hook-naming`](./src/rules/hook-naming.md) | Ensure that hooks returned by the `makeStyles()` function start with "use" | ❌ | | [`@griffel/no-invalid-shorthand-arguments`](./src/rules/no-invalid-shorthand-arguments.md) | All shorthands must not use space separators, and instead pass in multiple arguments | ✅ | | [`@griffel/no-shorthands`](./src/rules/no-shorthands.md) | Enforce usage of CSS longhands | ✅ | -| [`@griffel/styles-file`](./src/rules/styles-file.md) | Ensures that all `makeStyles()` and `makeResetStyles()` calls are placed in a `.styles.js` or `.styles.ts` file | ❌ | +| [`@griffel/styles-file`](./src/rules/styles-file.md) | Ensures that all `makeStyles()` and `makeResetStyles()` calls are placed in a `.styles.js` or `.styles.ts` file | ❌ | limitations) | ❌ | | [`@griffel/pseudo-element-naming`](./src/rules/pseudo-element-naming.md) | Ensures that all Pseudo Elements start with two colons | ✅ | +| [`@griffel/top-level-styles`](./src/rules/top-level-styles.md) | Ensures that all `makeStyles()`, `makeResetStyles()` and `makeStaticStyles()` calls are written in the top level of a file to discourage developer misuse. | ❌ | diff --git a/packages/eslint-plugin/src/rules/styles-file.ts b/packages/eslint-plugin/src/rules/styles-file.ts index 17ae554be..39bbe868f 100644 --- a/packages/eslint-plugin/src/rules/styles-file.ts +++ b/packages/eslint-plugin/src/rules/styles-file.ts @@ -1,46 +1,14 @@ import { ESLintUtils } from '@typescript-eslint/utils'; -import type { TSESTree } from '@typescript-eslint/utils'; -import { isMakeStylesCallExpression } from '../utils/helpers'; +import { isMakeStylesCallExpression, isMakeStylesImport } from '../utils/helpers'; import { getDocsUrl } from '../utils/getDocsUrl'; -const MATCHING_PACKAGES = new Set(['@fluentui/react-components', '@griffel/core', '@griffel/react']); const STYLES_FILE_PATTERN = /^.*\.(styles)\.[j|t]s$/; -function isMatchingPackage(packageName: string) { - return MATCHING_PACKAGES.has(packageName); -} - function isStylesFile(fileName: string) { return STYLES_FILE_PATTERN.test(fileName); } -/** - * @param node - import node from AST - * @returns true if makeStyles is imported, or if the entire library is imported. Otherwise returns false - */ -function isMakeStylesImport(node: TSESTree.ImportDeclaration) { - return ( - isMatchingPackage(node.source.value) && - node.specifiers.filter(specifier => { - // import * as ... from - if (specifier.type === 'ImportNamespaceSpecifier') { - return true; - } - - if ('imported' in specifier) { - return ( - specifier.imported.name === 'makeStyles' || // import { makeStyles } from - specifier.imported.name === 'makeStaticStyles' || // import { makeStaticStyles } from - specifier.imported.name === 'makeResetStyles' // import { makeResetStyles } from - ); - } - - return false; - }).length > 0 - ); -} - export const RULE_NAME = 'styles-file'; export const stylesFileRule = ESLintUtils.RuleCreator(getDocsUrl)({ diff --git a/packages/eslint-plugin/src/rules/top-level-styles.md b/packages/eslint-plugin/src/rules/top-level-styles.md new file mode 100644 index 000000000..182284fd0 --- /dev/null +++ b/packages/eslint-plugin/src/rules/top-level-styles.md @@ -0,0 +1,62 @@ +# Ensure `makeStyles`, `makeResetStyles` and `makeStaticStyles` are only called at the top-level of a file + +This rule discourages developers from misusing the `makeStyles`, `makeResetStyles` and `makeStaticStyles` methods. These methods don't allow passing runtime values for constructing the styles, as described here under [limitations](https://griffel.js.org/react/guides/). To encourage proper usage, this rule only permits calling those methods in the _top-level scope_ of a file, where it's far less likely developers will pass runtime values. + +## Rule Details + +Examples of **incorrect** code for this rule: + +```js +// filename: component.tsx +import { makeStyles } from '@griffel/react'; + +export const getStyles = () => { + const useStyles = makeStyles({ + root: { + backgroundColor: 'red', + }, + }); + + return useStyles; +} +``` + +```js +// filename: component.tsx +import { makeStyles } from '@fluentui/react-components'; + +export class MyClass { + getStyles () { + const styles = makeStyles({ + root: { + backgroundColor: 'red', + }, + }); + + return styles; + } +} +``` + +Examples of **correct** code for this rule: + +```js +import { makeStyles } from '@griffel/react'; + +export const useStyles = makeStyles({ + root: { + backgroundColor: 'red', + }, +}); +``` + +```js +import { makeStyles } from '@fluentui/react-components'; +import { generateStyles } from './custom-css'; + +export const useStyles = generateStyles(makeStyles({ + root: { + backgroundColor: 'red', + }, +})); +``` diff --git a/packages/eslint-plugin/src/rules/top-level-styles.test.ts b/packages/eslint-plugin/src/rules/top-level-styles.test.ts new file mode 100644 index 000000000..80b4e45b6 --- /dev/null +++ b/packages/eslint-plugin/src/rules/top-level-styles.test.ts @@ -0,0 +1,116 @@ +import { TSESLint } from '@typescript-eslint/utils'; +import { stylesFileRule, RULE_NAME } from './top-level-styles'; +import * as path from 'path'; + +const componentFileName = 'packages/components/components-foo/foo.tsx'; + +const ruleTester = new TSESLint.RuleTester({ + parser: path.resolve('./node_modules/@typescript-eslint/parser/dist'), + parserOptions: { + ecmaVersion: 2018, + sourceType: 'module', + }, +}); + +ruleTester.run(RULE_NAME, stylesFileRule, { + valid: [ + { + code: ` +import { makeStyles } from '@fluentui/react-components'; + +export const useStyles = makeStyles({ + root: { color: 'blue' }, +}); +`, + filename: componentFileName, + }, + { + code: ` +import { makeStyles } from 'unknown-package'; + +export function getStyles() { + const useStyles = makeStyles({ + root: { color: 'blue' }, + }); + + return useStyles; +} +`, + filename: componentFileName, + }, + ], + + invalid: [ + { + code: ` + import { makeStyles } from '@fluentui/react-components'; + function getStyles() { + const useStyles = makeStyles({ + root: { color: 'blue' }, + }); + + return useStyles; + } + `, + errors: [{ messageId: 'foundInvalidUsage', data: { methodName: 'makeStyles' } }], + filename: componentFileName, + }, + { + code: ` + import { makeStaticStyles } from '@fluentui/react-components'; + function getStyles() { + const useStyles = makeStaticStyles({ + root: { color: 'blue' }, + }); + + return useStyles; + } + `, + errors: [{ messageId: 'foundInvalidUsage', data: { methodName: 'makeStaticStyles' } }], + filename: componentFileName, + }, + { + code: ` + import { makeResetStyles } from '@fluentui/react-components'; + function getStyles() { + const useStyles = makeResetStyles({ + root: { color: 'blue' }, + }); + + return useStyles; + } + `, + errors: [{ messageId: 'foundInvalidUsage', data: { methodName: 'makeResetStyles' } }], + filename: componentFileName, + }, + { + code: ` + import { makeStyles } from '@griffel/react'; + const getStyles = () => { + const useStyles = makeStyles({ + root: { color: 'blue' }, + }); + + return useStyles; + }; + `, + errors: [{ messageId: 'foundInvalidUsage', data: { methodName: 'makeStyles' } }], + filename: componentFileName, + }, + { + code: ` + import { makeStyles } from '@fluentui/react-components'; + + class MyClass { + constructor() { + const useStyles = makeStyles({ + root: { color: 'blue' }, + }); + } + } + `, + errors: [{ messageId: 'foundInvalidUsage', data: { methodName: 'makeStyles' } }], + filename: componentFileName, + }, + ], +}); diff --git a/packages/eslint-plugin/src/rules/top-level-styles.ts b/packages/eslint-plugin/src/rules/top-level-styles.ts new file mode 100644 index 000000000..39f5a88a0 --- /dev/null +++ b/packages/eslint-plugin/src/rules/top-level-styles.ts @@ -0,0 +1,51 @@ +import { ESLintUtils } from '@typescript-eslint/utils'; + +import { getMakeStylesCallExpression, isMakeStylesImport } from '../utils/helpers'; +import { getDocsUrl } from '../utils/getDocsUrl'; + +export const RULE_NAME = 'top-level-styles'; + +export const stylesFileRule = ESLintUtils.RuleCreator(getDocsUrl)({ + name: RULE_NAME, + meta: { + docs: { + description: + 'Ensure makeStyles(), makeResetStyles() and makeStaticStyles() calls are placed in the top level of the file', + recommended: 'recommended', + }, + messages: { + foundInvalidUsage: '`{{ methodName }}` should be only be called from the top-level of the file', + }, + type: 'problem', + schema: [], + }, + defaultOptions: [], + create(context) { + let isMakeStylesImported = false; + + return { + ImportDeclaration(node) { + if (!isMakeStylesImported) { + isMakeStylesImported = isMakeStylesImport(node); + } + }, + + 'FunctionDeclaration CallExpression, ArrowFunctionExpression CallExpression, FunctionExpression CallExpression': + function (node) { + if (!isMakeStylesImported) { + return; + } + const methodName = getMakeStylesCallExpression(node, 'makeStyles', 'makeStaticStyles', 'makeResetStyles'); + if (methodName) { + context.report({ + messageId: 'foundInvalidUsage', + data: { + methodName, + }, + node, + }); + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/src/utils/helpers.ts b/packages/eslint-plugin/src/utils/helpers.ts index d7b8a9880..2bc63248d 100644 --- a/packages/eslint-plugin/src/utils/helpers.ts +++ b/packages/eslint-plugin/src/utils/helpers.ts @@ -34,20 +34,62 @@ export function isMakeStylesCallExpression( node: TSESTree.CallExpression, ...functionNames: [MakeStylesName, ...MakeStylesName[]] ): boolean { + return getMakeStylesCallExpression(node, ...functionNames) !== null; +} + +export function getMakeStylesCallExpression( + node: TSESTree.CallExpression, + ...functionNames: [MakeStylesName, ...MakeStylesName[]] +) { const { callee } = node; const names: ReadonlySet = new Set(functionNames); - if (isIdentifier(callee)) { + if (isIdentifier(callee) && names.has(callee.name)) { // makeStyles({}) - return names.has(callee.name); + return callee.name; } else if (isMemberExpression(callee)) { - return ( - // something.makeStyles({}) - (isIdentifier(callee.property) && names.has(callee.property.name)) || - // something['makeStyles']({}) - (isStringLiteral(callee.property) && names.has(callee.property.value)) - ); - } else { - return false; + // something.makeStyles({}) + if (isIdentifier(callee.property) && names.has(callee.property.name)) { + return callee.property.name; + } + + // something['makeStyles']({}) + if (isStringLiteral(callee.property) && names.has(callee.property.value)) { + return callee.property.value; + } } + + return null; +} + +const MATCHING_PACKAGES = new Set(['@fluentui/react-components', '@griffel/core', '@griffel/react']); + +function isMatchingPackage(packageName: string) { + return MATCHING_PACKAGES.has(packageName); +} + +/** + * @param node - import node from AST + * @returns true if makeStyles is imported, or if the entire library is imported. Otherwise returns false + */ +export function isMakeStylesImport(node: TSESTree.ImportDeclaration) { + return ( + isMatchingPackage(node.source.value) && + node.specifiers.filter(specifier => { + // import * as ... from + if (specifier.type === 'ImportNamespaceSpecifier') { + return true; + } + + if ('imported' in specifier) { + return ( + specifier.imported.name === 'makeStyles' || // import { makeStyles } from + specifier.imported.name === 'makeStaticStyles' || // import { makeStaticStyles } from + specifier.imported.name === 'makeResetStyles' // import { makeResetStyles } from + ); + } + + return false; + }).length > 0 + ); }