From 4dd1eb1d1d0e3f4dd28d1cdae35cbeaa60e4c249 Mon Sep 17 00:00:00 2001 From: Rel1cx Date: Fri, 4 Apr 2025 00:46:39 +0800 Subject: [PATCH 1/2] refactor: code optimizations --- ...dangerously-set-innerhtml-with-children.ts | 3 +- .../src/rules/no-useless-fragment.spec.ts | 291 +++++++----------- .../src/rules/no-useless-fragment.ts | 39 ++- packages/utilities/ast/src/index.ts | 1 + packages/utilities/ast/src/is-line-break.ts | 16 + packages/utilities/jsx/src/index.ts | 3 +- packages/utilities/jsx/src/is-jsx-fragment.ts | 13 + packages/utilities/jsx/src/is-jsx-text.ts | 12 + .../utilities/jsx/src/is-kind-of-element.ts | 6 +- packages/utilities/jsx/src/is-literal.ts | 41 --- 10 files changed, 196 insertions(+), 229 deletions(-) create mode 100644 packages/utilities/ast/src/is-line-break.ts create mode 100644 packages/utilities/jsx/src/is-jsx-fragment.ts create mode 100644 packages/utilities/jsx/src/is-jsx-text.ts delete mode 100644 packages/utilities/jsx/src/is-literal.ts diff --git a/packages/plugins/eslint-plugin-react-dom/src/rules/no-dangerously-set-innerhtml-with-children.ts b/packages/plugins/eslint-plugin-react-dom/src/rules/no-dangerously-set-innerhtml-with-children.ts index df159017b0..d86194f74c 100644 --- a/packages/plugins/eslint-plugin-react-dom/src/rules/no-dangerously-set-innerhtml-with-children.ts +++ b/packages/plugins/eslint-plugin-react-dom/src/rules/no-dangerously-set-innerhtml-with-children.ts @@ -2,6 +2,7 @@ import type { RuleContext, RuleFeature } from "@eslint-react/kit"; import type { TSESTree } from "@typescript-eslint/types"; import type { RuleListener } from "@typescript-eslint/utils/ts-eslint"; import type { CamelCase } from "string-ts"; +import * as AST from "@eslint-react/ast"; import * as JSX from "@eslint-react/jsx"; import { createRule } from "../utils"; @@ -51,5 +52,5 @@ export function create(context: RuleContext): RuleListener { function hasChildrenWithin(node: TSESTree.JSXElement): boolean { return node.children.length > 0 && node.children[0] != null - && !JSX.isLineBreak(node.children[0]); + && !AST.isLineBreak(node.children[0]); } diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.spec.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.spec.ts index 3fc3544909..628e9159b7 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.spec.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.spec.ts @@ -7,84 +7,85 @@ import rule, { RULE_NAME } from "./no-useless-fragment"; ruleTester.run(RULE_NAME, rule, { invalid: [ { - code: tsx`<>`, - errors: [{ - type: T.JSXFragment, - messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, - }], - output: null, - }, - { - code: tsx`

<>foo

`, + code: "<>", errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "contains less than two children" }, }, + ], + output: null, + }, + { + code: "<>{}", + errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], - output: tsx`

foo

`, + options: [{ allowExpressions: false }], + output: null, }, { - code: tsx`

moo<>foo

`, + code: "

moo<>foo

", errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, }, { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], output: "

moofoo

", }, { - code: tsx`

<>{meow}

`, + code: "<>{meow}", + errors: [ + { + type: T.JSXFragment, + messageId: "uselessFragment", + data: { reason: "contains less than two children" }, + }, + ], + options: [{ allowExpressions: false }], + output: null, + }, + { + code: "

<>{meow}

", errors: [ + // { + // type: T.JSXFragment, + // messageId: "uselessFragment", + // data: { reason: "contains less than two children" }, + // }, { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, }, ], output: "

{meow}

", }, { - code: tsx`<>
`, + code: "<>
", errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], - output: tsx`
`, + output: "
", }, { - code: tsx` + code: ` <>
@@ -93,30 +94,25 @@ ruleTester.run(RULE_NAME, rule, { { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], - output: tsx` + output: `
`, }, { - code: tsx``, + code: "", errors: [ { type: T.JSXElement, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], - output: null, }, { - code: tsx` + code: ` @@ -125,253 +121,186 @@ ruleTester.run(RULE_NAME, rule, { { type: T.JSXElement, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], - output: tsx` + output: ` `, }, + // { + // code: ` + // + // {foo} + // + // `, + // errors: [ + // { + // type: T.JSXElement, + // messageId: "uselessFragment", + // data: { reason: "contains less than two children" }, + // }, + // ], + // }, { - code: tsx`<>foo`, + // Not safe to fix this case because `Eeee` might require child be ReactElement + code: "<>foo", errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], output: null, }, { - code: tsx`
<>foo
`, + code: "
<>foo
", errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, }, { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], output: "
foo
", }, { - code: tsx`
<>{"a"}{"b"}
`, + code: '
<>{"a"}{"b"}
', errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, }, ], output: '
{"a"}{"b"}
', }, { - code: tsx` + code: `
<>{"a"}{"b"} -
- `, + `, errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, }, ], - output: tsx` + output: `
{"a"}{"b"} -
- `, + `, }, { - code: tsx`
{"a"}{"b"}
`, + code: '
{"a"}{"b"}
', errors: [ { type: T.JSXElement, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, }, ], output: '
{"a"}{"b"}
', }, { // whitespace tricky case - code: tsx` + code: `
git<> hub. git<> hub -
- `, + `, errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, + line: 3, }, { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, + line: 7, }, ], - output: tsx` + output: `
github. git hub -
- `, + `, }, { - code: tsx`
a <>{""}{""} a
`, + code: '
a <>{""}{""} a
', errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, }, ], output: '
a {""}{""} a
', }, { - code: tsx`const Comp = () => ();`, + code: ` + const Comp = () => ( + + + + ); + `, errors: [ { type: T.JSXElement, messageId: "uselessFragment", - data: { - reason: "placed inside a built-in component", - }, + data: { reason: "placed inside a built-in component" }, + line: 4, }, { type: T.JSXElement, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, + line: 4, }, ], - output: tsx`const Comp = () => ();`, + output: ` + const Comp = () => ( + + ${/* the trailing whitespace here is intentional */ ""} + + ); + `, }, // Ensure allowExpressions still catches expected violations { - code: tsx`<>{moo}`, - errors: [ - { - type: T.JSXFragment, - messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, - }, - ], - output: tsx`{moo}`, - }, - { - code: tsx`<>{moo}`, + code: "<>{moo}", errors: [ { type: T.JSXFragment, messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + data: { reason: "contains less than two children" }, }, ], - options: [{ allowExpressions: false }], - }, - { - code: tsx`<>{moo}`, - errors: [ - { - type: T.JSXFragment, - messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, - }, - ], - options: [{ allowExpressions: false }], - }, - { - code: tsx`<>{moo}`, - errors: [ - { - type: T.JSXElement, - messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, - }, - { - type: T.JSXFragment, - messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, - }, - ], - options: [{ allowExpressions: false }], - output: tsx`<>{moo}`, - }, - { - code: tsx`baz}/>`, - errors: [ - { - type: T.JSXFragment, - messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, - }, - ], - options: [{ allowExpressions: false }], - }, - { - code: tsx`<>`, - errors: [ + options: [ { - type: T.JSXFragment, - messageId: "uselessFragment", - data: { - reason: "contains less than two children", - }, + allowExpressions: true, }, ], - options: [{ allowExpressions: false }], + output: "{moo}", }, ], valid: [ @@ -421,6 +350,14 @@ ruleTester.run(RULE_NAME, rule, { {moo} `, + tsx` + const element = ( + <> + +   + + ); + `, tsx`<>{cloneElement(children, { ref: childrenRef })}`, { code: tsx` diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.ts index 0d44a79a6c..9a1e78ad33 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-useless-fragment.ts @@ -58,14 +58,41 @@ export function create(context: RuleContext, [option]: Optio return { JSXElement(node) { if (!JSX.isFragmentElement(node)) return; - doCheck(context, node, allowExpressions); + checkNode(context, node, allowExpressions); }, JSXFragment(node) { - doCheck(context, node, allowExpressions); + checkNode(context, node, allowExpressions); }, }; } +/** + * Check if a node is a Literal or JSXText + * @param node The AST node to check + * @returns boolean `true` if the node is a Literal or JSXText + */ +const isLiteral = AST.isOneOf([T.Literal, T.JSXText]); + +/** + * Check if a Literal or JSXText node is whitespace + * @param node The AST node to check + * @returns boolean `true` if the node is whitespace + */ +function isWhiteSpace(node: TSESTree.JSXText | TSESTree.Literal) { + return typeof node.value === "string" && node.raw.trim() === ""; +} + +/** + * Check if a Literal or JSXText node is padding spaces + * @param node The AST node to check + * @returns boolean + */ +function isPaddingSpaces(node: TSESTree.Node) { + return isLiteral(node) + && isWhiteSpace(node) + && node.raw.includes("\n"); +} + function trimLikeReact(text: string) { const leadingSpaces = /^\s*/.exec(text)?.[0] ?? ""; const trailingSpaces = /\s*$/.exec(text)?.[0] ?? ""; @@ -76,7 +103,7 @@ function trimLikeReact(text: string) { return text.slice(start, end); } -function doCheck( +function checkNode( context: RuleContext, node: TSESTree.JSXElement | TSESTree.JSXFragment, allowExpressions: boolean, @@ -93,7 +120,7 @@ function doCheck( if ( node.children.some( (child) => - (JSX.isLiteral(child) && !JSX.isWhiteSpace(child)) + (isLiteral(child) && !isWhiteSpace(child)) || AST.is(T.JSXExpressionContainer)(child), ) ) { @@ -150,7 +177,7 @@ function doCheck( case allowExpressions && !isChildElement && node.children.length === 1 - && JSX.isLiteral(node.children.at(0)): { + && isLiteral(node.children.at(0)): { return; } // <>hello, world @@ -182,7 +209,7 @@ function doCheck( return; } } - const nonPaddingChildren = node.children.filter((child) => !JSX.isPaddingSpaces(child)); + const nonPaddingChildren = node.children.filter((child) => !isPaddingSpaces(child)); const firstNonPaddingChild = nonPaddingChildren.at(0); switch (true) { case nonPaddingChildren.length === 0: diff --git a/packages/utilities/ast/src/index.ts b/packages/utilities/ast/src/index.ts index cdf8cc2958..d669cfa755 100644 --- a/packages/utilities/ast/src/index.ts +++ b/packages/utilities/ast/src/index.ts @@ -14,6 +14,7 @@ export * from "./is"; export * from "./is-array-map-call"; export * from "./is-empty-function"; export * from "./is-kind-of-literal"; +export * from "./is-line-break"; export * from "./is-multi-line"; export * from "./is-node-equal"; export * from "./is-this-expression"; diff --git a/packages/utilities/ast/src/is-line-break.ts b/packages/utilities/ast/src/is-line-break.ts new file mode 100644 index 0000000000..2aa5ade870 --- /dev/null +++ b/packages/utilities/ast/src/is-line-break.ts @@ -0,0 +1,16 @@ +import type { TSESTree } from "@typescript-eslint/types"; +import { AST_NODE_TYPES as T } from "@typescript-eslint/types"; +import { isOneOf } from "./is"; +import { isMultiLine } from "./is-multi-line"; + +/** + * Check if a node is a line break + * @param node The AST node to check + * @returns boolean + */ +export function isLineBreak(node: TSESTree.Node) { + return isOneOf([T.Literal, T.JSXText])(node) + && typeof node.value === "string" + && node.value.trim() === "" + && isMultiLine(node); +} diff --git a/packages/utilities/jsx/src/index.ts b/packages/utilities/jsx/src/index.ts index 3934cd459b..d742bb57e6 100644 --- a/packages/utilities/jsx/src/index.ts +++ b/packages/utilities/jsx/src/index.ts @@ -4,7 +4,8 @@ export * from "./get-attribute-name"; export * from "./get-attribute-value"; export * from "./get-element-type"; export * from "./has-attribute"; +export * from "./is-jsx-fragment"; +export * from "./is-jsx-text"; export * from "./is-jsx-value"; export * from "./is-kind-of-element"; -export * from "./is-literal"; export * from "./to-string"; diff --git a/packages/utilities/jsx/src/is-jsx-fragment.ts b/packages/utilities/jsx/src/is-jsx-fragment.ts new file mode 100644 index 0000000000..6d54c5d3d1 --- /dev/null +++ b/packages/utilities/jsx/src/is-jsx-fragment.ts @@ -0,0 +1,13 @@ +import type { _ } from "@eslint-react/eff"; +import { AST_NODE_TYPES, type TSESTree } from "@typescript-eslint/types"; +import { isFragmentElement } from "./is-kind-of-element"; + +/** + * Check if a node is a `JSXFragment` or a `Fragment` element + * @param node The AST node to check + * @returns `true` if the node is a `JSXFragment` or a `Fragment` element + */ +export function isJSXFragmentLike(node: TSESTree.Node | null | _) { + if (node == null) return false; + return node.type === AST_NODE_TYPES.JSXFragment || isFragmentElement(node); +} diff --git a/packages/utilities/jsx/src/is-jsx-text.ts b/packages/utilities/jsx/src/is-jsx-text.ts new file mode 100644 index 0000000000..c306f77bef --- /dev/null +++ b/packages/utilities/jsx/src/is-jsx-text.ts @@ -0,0 +1,12 @@ +import type { _ } from "@eslint-react/eff"; +import { AST_NODE_TYPES as T, type TSESTree } from "@typescript-eslint/types"; + +/** + * Check if a node is a `JSXText` or a `Literal` node + * @param node The AST node to check + * @returns `true` if the node is a `JSXText` or a `Literal` node + */ +export function isJSXTextLike(node: TSESTree.Node | null | _): node is TSESTree.JSXText | TSESTree.Literal { + if (node == null) return false; + return node.type === T.JSXText || node.type === T.Literal; +} diff --git a/packages/utilities/jsx/src/is-kind-of-element.ts b/packages/utilities/jsx/src/is-kind-of-element.ts index b60e806131..4a0696b613 100644 --- a/packages/utilities/jsx/src/is-kind-of-element.ts +++ b/packages/utilities/jsx/src/is-kind-of-element.ts @@ -14,7 +14,7 @@ export function isFragmentElement(node: TSESTree.Node) { if (node.type !== T.JSXElement) return false; return getElementType(node) .split(".") - .at(-1) === "Fragment"; + .at(-1)?.endsWith("Fragment"); } /** @@ -29,7 +29,7 @@ export function isKeyedElement(node: TSESTree.Node, initialScope?: Scope) { } /** - * Check if a node is a `JSXFragment` of `Built-in Component` type + * Check if a node is a `JSXElement` of built-in component * @param node The AST node to check * @returns `true` if the node is a `JSXFragment` of `Built-in Component` type */ @@ -41,7 +41,7 @@ export function isBuiltInElement(node: TSESTree.Node) { } /** - * Check if a node is a `JSXElement` of `User-Defined Component` type + * Check if a node is a `JSXElement` of user-defined component * @param node The AST node to check * @returns `true` if the node is a `JSXElement` of `User-Defined Component` type */ diff --git a/packages/utilities/jsx/src/is-literal.ts b/packages/utilities/jsx/src/is-literal.ts deleted file mode 100644 index fbd1df130f..0000000000 --- a/packages/utilities/jsx/src/is-literal.ts +++ /dev/null @@ -1,41 +0,0 @@ -import type { TSESTree } from "@typescript-eslint/types"; -import * as AST from "@eslint-react/ast"; -import { AST_NODE_TYPES as T } from "@typescript-eslint/types"; - -/** - * Check if a node is a Literal or JSXText - * @param node The AST node to check - * @returns boolean `true` if the node is a Literal or JSXText - */ -export const isLiteral = AST.isOneOf([T.Literal, T.JSXText]); - -/** - * Check if a Literal or JSXText node is whitespace - * @param node The AST node to check - * @returns boolean `true` if the node is whitespace - */ -export function isWhiteSpace(node: TSESTree.JSXText | TSESTree.Literal) { - return typeof node.value === "string" && node.value.trim() === ""; -} - -/** - * Check if a Literal or JSXText node is a line break - * @param node The AST node to check - * @returns boolean - */ -export function isLineBreak(node: TSESTree.Node) { - return isLiteral(node) - && isWhiteSpace(node) - && AST.isMultiLine(node); -} - -/** - * Check if a Literal or JSXText node is padding spaces - * @param node The AST node to check - * @returns boolean - */ -export function isPaddingSpaces(node: TSESTree.Node) { - return isLiteral(node) - && isWhiteSpace(node) - && node.raw.includes("\n"); -} From 06dab83f7b2b4b43442f0a1639ba15f615354cba Mon Sep 17 00:00:00 2001 From: Rel1cx Date: Fri, 4 Apr 2025 00:59:52 +0800 Subject: [PATCH 2/2] fix: fixed `no-useless-fragment` false positive when using ` ` --- packages/utilities/jsx/src/is-kind-of-element.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/utilities/jsx/src/is-kind-of-element.ts b/packages/utilities/jsx/src/is-kind-of-element.ts index 4a0696b613..0b6232dce1 100644 --- a/packages/utilities/jsx/src/is-kind-of-element.ts +++ b/packages/utilities/jsx/src/is-kind-of-element.ts @@ -14,7 +14,7 @@ export function isFragmentElement(node: TSESTree.Node) { if (node.type !== T.JSXElement) return false; return getElementType(node) .split(".") - .at(-1)?.endsWith("Fragment"); + .at(-1) === "Fragment"; } /**