diff --git a/.changeset/fix-expo-metro-config-subpaths.md b/.changeset/fix-expo-metro-config-subpaths.md new file mode 100644 index 0000000000..f6274e8080 --- /dev/null +++ b/.changeset/fix-expo-metro-config-subpaths.md @@ -0,0 +1,5 @@ +--- +"@react-doctor/core": patch +--- + +Keep `@expo/metro-config` as a direct dependency when project code imports a package subpath that the `expo/metro-config` umbrella does not expose. diff --git a/.changeset/fix-fbt-component-return.md b/.changeset/fix-fbt-component-return.md new file mode 100644 index 0000000000..191387ae05 --- /dev/null +++ b/.changeset/fix-fbt-component-return.md @@ -0,0 +1,6 @@ +--- +"oxlint-plugin-react-doctor": patch +"react-doctor": patch +--- + +Fix `rn-no-raw-text` false positives in components that return only direct `` or `` elements. diff --git a/.changeset/fix-fbt-in-text-wrappers.md b/.changeset/fix-fbt-in-text-wrappers.md new file mode 100644 index 0000000000..803977fcbf --- /dev/null +++ b/.changeset/fix-fbt-in-text-wrappers.md @@ -0,0 +1,7 @@ +--- +"oxlint-plugin-react-doctor": patch +"eslint-plugin-react-doctor": patch +"react-doctor": patch +--- + +Prevent `rn-no-raw-text` reports for `` content passed through verified React Native text wrappers. diff --git a/.changeset/fix-test-noise-application-paths.md b/.changeset/fix-test-noise-application-paths.md new file mode 100644 index 0000000000..737e87bef3 --- /dev/null +++ b/.changeset/fix-test-noise-application-paths.md @@ -0,0 +1,7 @@ +--- +"oxlint-plugin-react-doctor": patch +"eslint-plugin-react-doctor": patch +"react-doctor": patch +--- + +Run `test-noise` rules in ambiguous product-named directories such as `tools`, `demo`, and `migrations` when they are below a recognized application source root. Explicit test surfaces and root-level tooling or example directories remain excluded. diff --git a/packages/core/src/checks/expo/check-flagged-dependencies.ts b/packages/core/src/checks/expo/check-flagged-dependencies.ts index 6e39d02b6a..6c17250de3 100644 --- a/packages/core/src/checks/expo/check-flagged-dependencies.ts +++ b/packages/core/src/checks/expo/check-flagged-dependencies.ts @@ -1,6 +1,7 @@ import type { Diagnostic } from "../../types/index.js"; import type { ExpoCheckContext } from "./expo-check-context.js"; import { buildExpoDiagnostic } from "./utils/build-expo-diagnostic.js"; +import { hasStaticModuleSubpath } from "./utils/has-static-module-subpath.js"; import { isExpoSdkAtLeast } from "./utils/is-expo-sdk-at-least.js"; interface FlaggedDependency { @@ -8,6 +9,7 @@ interface FlaggedDependency { readonly rule: string; readonly message: string; readonly help: string; + readonly skipWhenSubpathImported?: boolean; /** * Lowest Expo SDK major the finding applies to. When set, the entry * stays quiet unless the resolved SDK major is known AND at least this @@ -95,6 +97,7 @@ const FLAGGED_DEPENDENCIES: ReadonlyArray = [ message: '"@expo/metro-config" should not be a direct dependency. Expo pins the compatible Metro config, and a direct entry can drift to a version that breaks bundling', help: "Remove `@expo/metro-config` and import `expo/metro-config` in your metro.config.js", + skipWhenSubpathImported: true, }, { packageName: "@types/react-native", @@ -165,8 +168,19 @@ const FLAGGED_DEPENDENCIES: ReadonlyArray = [ export const checkExpoFlaggedDependencies = (context: ExpoCheckContext): Diagnostic[] => FLAGGED_DEPENDENCIES.filter((flaggedDependency) => { if (!context.directDependencyNames.has(flaggedDependency.packageName)) return false; - if (flaggedDependency.minSdkMajor === undefined) return true; - return isExpoSdkAtLeast(context.expoSdkMajor, flaggedDependency.minSdkMajor); + if ( + flaggedDependency.minSdkMajor !== undefined && + !isExpoSdkAtLeast(context.expoSdkMajor, flaggedDependency.minSdkMajor) + ) { + return false; + } + if ( + flaggedDependency.skipWhenSubpathImported && + hasStaticModuleSubpath(context.rootDirectory, flaggedDependency.packageName) + ) { + return false; + } + return true; }).map((flaggedDependency) => buildExpoDiagnostic({ rule: flaggedDependency.rule, diff --git a/packages/core/src/checks/expo/utils/has-static-module-subpath.ts b/packages/core/src/checks/expo/utils/has-static-module-subpath.ts new file mode 100644 index 0000000000..78bda27d09 --- /dev/null +++ b/packages/core/src/checks/expo/utils/has-static-module-subpath.ts @@ -0,0 +1,33 @@ +import * as fs from "node:fs"; +import { collectStaticModuleSpecifiers } from "../../../project-analysis/utils/collect-static-module-specifiers.js"; +import { walkSourceTreeFiles } from "../../../utils/walk-source-tree-files.js"; + +const JAVASCRIPT_MODULE_FILE_PATTERN = /\.[cm]?[jt]sx?$/; + +export const hasStaticModuleSubpath = (rootDirectory: string, packageName: string): boolean => { + const packageSubpathPrefix = `${packageName}/`; + + for (const { absolutePath, name } of walkSourceTreeFiles(rootDirectory)) { + if (!JAVASCRIPT_MODULE_FILE_PATTERN.test(name)) continue; + + let sourceText: string; + try { + sourceText = fs.readFileSync(absolutePath, "utf-8"); + } catch { + continue; + } + if (!sourceText.includes(packageSubpathPrefix)) continue; + + let moduleSpecifiers: Set; + try { + moduleSpecifiers = collectStaticModuleSpecifiers(sourceText, { filePath: absolutePath }); + } catch { + continue; + } + for (const moduleSpecifier of moduleSpecifiers) { + if (moduleSpecifier.startsWith(packageSubpathPrefix)) return true; + } + } + + return false; +}; diff --git a/packages/core/tests/check-expo-project.test.ts b/packages/core/tests/check-expo-project.test.ts index 445d890627..a1198b6d01 100644 --- a/packages/core/tests/check-expo-project.test.ts +++ b/packages/core/tests/check-expo-project.test.ts @@ -165,6 +165,78 @@ describe("checkExpoProject — redundant transitive dependencies", () => { ), ).toHaveLength(1); }); + + it("keeps @expo/metro-config when a package subpath is imported", () => { + const projectDirectory = makeProjectDirectory(); + writePackageJson(projectDirectory, { + name: "expo-app", + dependencies: { + expo: "~57.0.18", + "@expo/metro-config": "57.0.12", + }, + }); + writeFile( + projectDirectory, + "metro.transformer.cjs", + `const upstreamTransformer = require("@expo/metro-config/babel-transformer");`, + ); + + const diagnostics = checkExpoProject( + projectDirectory, + buildExpoProject(projectDirectory, "~57.0.18"), + ); + expect( + rulesOf(diagnostics).filter((rule) => rule === "expo-no-redundant-dependency"), + ).toHaveLength(0); + }); + + it("still flags @expo/metro-config when only the package root is imported", () => { + const projectDirectory = makeProjectDirectory(); + writePackageJson(projectDirectory, { + name: "expo-app", + dependencies: { + expo: "~57.0.18", + "@expo/metro-config": "57.0.12", + }, + }); + writeFile( + projectDirectory, + "metro.config.js", + `const { getDefaultConfig } = require("@expo/metro-config");`, + ); + + const diagnostics = checkExpoProject( + projectDirectory, + buildExpoProject(projectDirectory, "~57.0.18"), + ); + expect( + rulesOf(diagnostics).filter((rule) => rule === "expo-no-redundant-dependency"), + ).toHaveLength(1); + }); + + it("still flags @expo/metro-config when a package subpath only appears in a comment", () => { + const projectDirectory = makeProjectDirectory(); + writePackageJson(projectDirectory, { + name: "expo-app", + dependencies: { + expo: "~57.0.18", + "@expo/metro-config": "57.0.12", + }, + }); + writeFile( + projectDirectory, + "metro.config.js", + `// require("@expo/metro-config/babel-transformer");\nmodule.exports = {};`, + ); + + const diagnostics = checkExpoProject( + projectDirectory, + buildExpoProject(projectDirectory, "~57.0.18"), + ); + expect( + rulesOf(diagnostics).filter((rule) => rule === "expo-no-redundant-dependency"), + ).toHaveLength(1); + }); }); describe("checkExpoProject — dependency overrides", () => { diff --git a/packages/fuzz/corpus/regressions/rn-no-raw-text--transparent-return.tsx b/packages/fuzz/corpus/regressions/rn-no-raw-text--transparent-return.tsx new file mode 100644 index 0000000000..e8055a28a6 --- /dev/null +++ b/packages/fuzz/corpus/regressions/rn-no-raw-text--transparent-return.tsx @@ -0,0 +1,17 @@ +// rule: rn-no-raw-text +// verdict: pass +// weakness: wrapper-transparency +// source: issue #1729 + +import { Text } from "react-native"; + +export const FbtLabel = () => Travel with confidence; + +export const StringLabel = () => "Travel with confidence"; + +export const Screen = () => ( + + + + +); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/constants/react-native.ts b/packages/oxlint-plugin-react-doctor/src/plugin/constants/react-native.ts index eeab08194a..7bda430a00 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/constants/react-native.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/constants/react-native.ts @@ -56,7 +56,12 @@ export const REACT_NATIVE_TEXT_COMPONENT_KEYWORDS = new Set([ // whether they wrap children in a is a per-project provider choice, so // they belong in an opt-in `transparentComponents` config instead. // Ref: https://github.com/millionco/react-doctor/issues/581 -export const REACT_NATIVE_TEXT_TRANSPARENT_COMPONENTS = new Set(["Fragment", "fbt", "fbs"]); +export const REACT_NATIVE_TRANSLATION_TEXT_COMPONENTS = new Set(["fbt", "fbs"]); + +export const REACT_NATIVE_TEXT_TRANSPARENT_COMPONENTS = new Set([ + "Fragment", + ...REACT_NATIVE_TRANSLATION_TEXT_COMPONENTS, +]); // HACK: Maps (not plain objects) so that an unusual `import { constructor } // from "react-native"` (or any other Object.prototype name) doesn't fall diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/a11y/no-placeholder-only-field.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/a11y/no-placeholder-only-field.test.ts index 23e092cb40..657a35bd4a 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/a11y/no-placeholder-only-field.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/a11y/no-placeholder-only-field.test.ts @@ -320,12 +320,12 @@ describe("no-placeholder-only-field", () => { expect(result.diagnostics).toHaveLength(1); }); - it("does not report placeholder-only fields in non-production files", () => { + it("reports placeholder-only fields in application demo directories", () => { const result = runRule( noPlaceholderOnlyField, `const Example = () => ;`, { filename: "src/demo/example.tsx" }, ); - expect(result.diagnostics).toHaveLength(0); + expect(result.diagnostics).toHaveLength(1); }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/no-create-object-url-without-revoke.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/no-create-object-url-without-revoke.test.ts index b18926a934..fc4256800d 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/no-create-object-url-without-revoke.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/no-create-object-url-without-revoke.test.ts @@ -246,13 +246,13 @@ describe("no-create-object-url-without-revoke", () => { expect(result.diagnostics).toHaveLength(0); }); - it("stays quiet in a demo file", () => { + it("reports in application demo directories", () => { const result = runRule( noCreateObjectUrlWithoutRevoke, `export default () => download;`, { filename: "/src/demos/index.tsx" }, ); - expect(result.diagnostics).toHaveLength(0); + expect(result.diagnostics).toHaveLength(1); }); it("stays quiet when URL is a local binding, not the DOM global", () => { diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-a-element.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-a-element.regressions.test.ts index 3d6e754811..a63a15eff7 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-a-element.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-a-element.regressions.test.ts @@ -23,6 +23,30 @@ describe("nextjs/nextjs-no-a-element — regressions", () => { expect(result.diagnostics.length).toBeGreaterThan(0); }); + it.each([ + "src/components/tools/widget.tsx", + "src/components/demo/widget.tsx", + "src/migrations/widget.tsx", + ])("still flags an internal route in application code at %s", (filename) => { + const result = runRule( + nextjsNoAElement, + `export default function C() { return About; }`, + { filename }, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); + + it("stays silent for an internal route in a root-level tooling directory", () => { + const result = runRule( + nextjsNoAElement, + `export default function C() { return About; }`, + { filename: "tools/widget.tsx" }, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + it("stays silent on a download anchor", () => { const result = runRule( nextjsNoAElement, diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.test.ts index 8a9fe7551e..8b3a29a579 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.test.ts @@ -554,6 +554,128 @@ describe("react-native/rn-no-raw-text", () => { ); `); }); + + it("does not fire on fbt inside an auto-detected text wrapper (issue #1722)", () => { + expectPass(` + const Card = ({ children }) => {children}; + const App = () => ( + + Fbt in Card + + ); + `); + }); + + it("does not fire on fbt inside a text-named wrapper (issue #1722)", () => { + expectPass(` + const Button = ({ children }) => {children}; + const App = () => ( + + ); + `); + }); + + it("does not fire on fbt passed as a prop to a text wrapper (issue #1722)", () => { + expectPass(` + const Card = ({ title }) => {title}; + const App = () => Scrollable content} />; + `); + }); + + it("still fires on fbt inside an auto-detected non-text wrapper", () => { + expectFail(` + const Box = ({ children }) => {children}; + const App = () => ( + + Hello + + ); + `); + }); + + it("does not fire on a component whose direct return is fbt (issue #1729)", () => { + expectPass(` + const FbtLabel = () => Travel with confidence; + const StringLabel = () => "Travel with confidence"; + const Screen = () => ( + + + + + ); + `); + }); + + it("does not fire when every direct JSX return is fbt or fbs", () => { + expectPass(` + const MotivationLabel = ({ motivation }) => { + switch (motivation) { + case "travel": { + return Travel with confidence; + } + case "career": { + return Grow my career; + } + } + }; + const Screen = () => ( + + + + ); + `); + }); + + it("does not fire on an HOC-wrapped function returning fbt", () => { + expectPass(` + const FbtLabel = memo(() => Travel with confidence); + const Screen = () => ( + + + + ); + `); + }); + + it("still fires on fbt inside a component that also returns non-transparent elements", () => { + expectFail(` + const MixedComponent = ({ type }) => { + if (type === "fbt") { + return Text; + } + return Non-text; + }; + const Screen = () => ( + + + + ); + `); + }); + + it("still fires when fbt is nested under a fragment return", () => { + expectFail(` + const FbtMarker = () => ( + + Travel with confidence + + ); + const Screen = () => ; + `); + }); + + it("does not classify a direct fbt return as a Text wrapper", () => { + expectFail(` + const FbtMarker = ({ children }) => Label; + const Screen = () => ( + + Nested label + + ); + `); + }); }); describe("test-noise suppression", () => { diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.ts index e52932deee..63470e7a67 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/react-native/rn-no-raw-text.ts @@ -19,6 +19,7 @@ import { resolveImportedComponentForwarding } from "../../utils/resolve-imported import { isExpoUiComponentElement } from "./utils/is-expo-ui-component-element.js"; import { isNodeOfType } from "../../utils/is-node-of-type.js"; import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; +import { enclosingComponentOrHookName } from "../../utils/enclosing-component-or-hook-name.js"; const truncateText = (text: string): string => { const collapsedText = text.replace(/\s+/g, " "); @@ -80,25 +81,6 @@ const isTextHandlingComponent = (elementName: string): boolean => { const isTransparentTextWrapper = (elementName: string | null): boolean => elementName !== null && REACT_NATIVE_TEXT_TRANSPARENT_COMPONENTS.has(elementName); -// Walks ancestors to a real text component, stepping through transparent -// wrappers. Returns false as soon as a non-transparent, non-text element -// breaks the chain — so the text boundary is only honored when every link -// up to the is itself transparent. -const isInsideTextHandlingComponent = (node: EsTreeNodeOfType<"JSXElement">): boolean => { - let parentNode = node.parent; - while (parentNode) { - if (!isNodeOfType(parentNode, "JSXElement")) { - parentNode = parentNode.parent; - continue; - } - const parentName = resolveTextBoundaryName(parentNode.openingElement); - if (parentName && isTextHandlingComponent(parentName)) return true; - if (!isTransparentTextWrapper(parentName)) return false; - parentNode = parentNode.parent; - } - return false; -}; - export const rnNoRawText = defineRule({ id: "rn-no-raw-text", title: "Raw text outside a Text component", @@ -124,6 +106,7 @@ export const rnNoRawText = defineRule({ // the rest (`node_modules` and anything the resolver can't follow). let autoDetectedTextWrappers: ReadonlySet = new Set(); let autoDetectedNonTextWrappers: ReadonlySet = new Set(); + let autoDetectedTranslationTextReturnComponents: ReadonlySet = new Set(); // A built-in crash host: a React Native host primitive, or a lowercase // intrinsic that is NOT a known HTML/SVG tag (`fbt`, a typo'd primitive). @@ -148,17 +131,11 @@ export const rnNoRawText = defineRule({ elementName !== null && (isNonTextHostName(elementName) || autoDetectedNonTextWrappers.has(elementName)); - // Resolve an imported component cross-file: "nonText" (renders children into - // a host) → reported; "text" or unresolvable (`node_modules`, namespace - // imports, shadowed bindings, unanalyzable exports) → left alone. - const isImportedNonTextWrapper = ( - elementName: string | null, - contextNode: EsTreeNode, - ): boolean => { - if (elementName === null || !isReactComponentName(elementName)) return false; + const resolveImportedWrapper = (elementName: string | null, contextNode: EsTreeNode) => { + if (elementName === null || !isReactComponentName(elementName)) return null; const { filename } = context; - if (filename === undefined) return false; - const forwardingKind = resolveImportedComponentForwarding( + if (filename === undefined) return null; + return resolveImportedComponentForwarding( contextNode, context.scopes, filename, @@ -166,7 +143,33 @@ export const rnNoRawText = defineRule({ isTextHandlingComponent, isNonTextHostName, ); - return forwardingKind === "nonText"; + }; + + const isImportedNonTextWrapper = ( + elementName: string | null, + contextNode: EsTreeNode, + ): boolean => resolveImportedWrapper(elementName, contextNode) === "nonText"; + + const isInsideTextHandlingComponent = (node: EsTreeNodeOfType<"JSXElement">): boolean => { + let parentNode = node.parent; + while (parentNode) { + if (!isNodeOfType(parentNode, "JSXElement")) { + parentNode = parentNode.parent; + continue; + } + const parentName = resolveTextBoundaryName(parentNode.openingElement); + if ( + parentName && + (isTextHandlingComponent(parentName) || + autoDetectedTextWrappers.has(parentName) || + resolveImportedWrapper(parentName, parentNode) === "text") + ) { + return true; + } + if (!isTransparentTextWrapper(parentName)) return false; + parentNode = parentNode.parent; + } + return false; }; return { @@ -179,6 +182,8 @@ export const rnNoRawText = defineRule({ ); autoDetectedTextWrappers = childrenForwarding.textWrappers; autoDetectedNonTextWrappers = childrenForwarding.nonTextWrappers; + autoDetectedTranslationTextReturnComponents = + childrenForwarding.translationTextReturnComponents; }, JSXElement(node: EsTreeNodeOfType<"JSXElement">) { if (isDomComponentFile) return; @@ -216,6 +221,13 @@ export const rnNoRawText = defineRule({ return; } + if ( + isTransparentTextWrapper(elementName) && + autoDetectedTranslationTextReturnComponents.has(enclosingComponentOrHookName(node) ?? "") + ) { + return; + } + // The cross-file lookup is the one expensive step, so gate it behind the // raw-text check and the cheap built-in/in-file checks. if (!(node.children ?? []).some(isRawTextContent)) return; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-text-wrapper-components.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-text-wrapper-components.ts index 5336ab8c19..8056767813 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-text-wrapper-components.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/collect-text-wrapper-components.ts @@ -1,5 +1,6 @@ import type { EsTreeNode } from "./es-tree-node.js"; import type { EsTreeNodeOfType } from "./es-tree-node-of-type.js"; +import { REACT_NATIVE_TRANSLATION_TEXT_COMPONENTS } from "../constants/react-native.js"; import { isJsxFragmentElement } from "./is-jsx-fragment-element.js"; import { isNodeOfType } from "./is-node-of-type.js"; import { isReactComponentName } from "./is-react-component-name.js"; @@ -407,11 +408,27 @@ const resolveClassRenderFunction = (classNode: EsTreeNode): FunctionNode | null return null; }; +const returnsOnlyTranslationTextElements = (definitionNode: EsTreeNode): boolean => { + const unwrapped = unwrapComponentDefinition(definitionNode); + if (!isFunctionNode(unwrapped)) return false; + const jsxRoots = collectReturnedJsxRoots(unwrapped); + return ( + jsxRoots.length > 0 && + jsxRoots.every((jsxRoot) => { + if (!isNodeOfType(jsxRoot, "JSXElement")) return false; + const rootName = resolveJsxElementName(jsxRoot.openingElement); + return rootName !== null && REACT_NATIVE_TRANSLATION_TEXT_COMPONENTS.has(rootName); + }) + ); +}; + export interface ChildrenForwardingComponents { // Forward their children into a `` — raw text inside them is safe. textWrappers: ReadonlySet; // Proven to render their children into a non-text host. nonTextWrappers: ReadonlySet; + // Return only direct translation text elements such as `` or ``. + translationTextReturnComponents: ReadonlySet; } interface ComponentDeclaration { @@ -519,6 +536,7 @@ export const collectTextWrapperComponents = ( ): ChildrenForwardingComponents => { const wrappers = new Set(); const nonTextWrappers = new Set(); + const translationTextReturnComponents = new Set(); const componentBindingCounts = new Map(); const componentDeclarations: ComponentDeclaration[] = []; let didContainJsxElement = false; @@ -553,7 +571,9 @@ export const collectTextWrapperComponents = ( componentDeclarations.push({ componentName, definitionNode: node }); } }); - if (!didContainJsxElement) return { textWrappers: wrappers, nonTextWrappers }; + if (!didContainJsxElement) { + return { textWrappers: wrappers, nonTextWrappers, translationTextReturnComponents }; + } const isTextHandlingElement = (elementName: string, contextNode: EsTreeNode): boolean => isTextHandlingRoot(elementName, contextNode) || wrappers.has(elementName); const isNonTextHostElement = (elementName: string, contextNode: EsTreeNode): boolean => @@ -571,6 +591,15 @@ export const collectTextWrapperComponents = ( ); }; + for (const declaration of componentDeclarations) { + if ( + componentBindingCounts.get(declaration.componentName) === 1 && + returnsOnlyTranslationTextElements(declaration.definitionNode) + ) { + translationTextReturnComponents.add(declaration.componentName); + } + } + while (true) { const wrappersSizeBeforePass = wrappers.size; const nonTextSizeBeforePass = nonTextWrappers.size; @@ -587,5 +616,5 @@ export const collectTextWrapperComponents = ( for (const wrapperName of wrappers) nonTextWrappers.delete(wrapperName); - return { textWrappers: wrappers, nonTextWrappers }; + return { textWrappers: wrappers, nonTextWrappers, translationTextReturnComponents }; }; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.test.ts index cf2c1e2e11..e70f469ba9 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.test.ts @@ -59,6 +59,62 @@ describe("defineRule", () => { expect(visitors.JSXOpeningElement).toBeTypeOf("function"); }); + it("keeps test-noise visitors in product directories below a source root", () => { + let didCreateVisitors = false; + const rule = defineRule({ + id: "test-noise-rule", + title: "test", + severity: "warn", + tags: ["test-noise"], + create: () => { + didCreateVisitors = true; + return { Program: () => {} }; + }, + }); + + const visitors = rule.create({ + filename: "src/components/tools/widget.tsx", + report: () => {}, + get scopes(): never { + throw new Error("scopes should stay lazy"); + }, + get cfg(): never { + throw new Error("cfg should stay lazy"); + }, + }); + + expect(didCreateVisitors).toBe(true); + expect(visitors.Program).toBeTypeOf("function"); + }); + + it("skips test-noise visitors in root-level tooling directories", () => { + let didCreateVisitors = false; + const rule = defineRule({ + id: "test-noise-rule", + title: "test", + severity: "warn", + tags: ["test-noise"], + create: () => { + didCreateVisitors = true; + return { Program: () => {} }; + }, + }); + + const visitors = rule.create({ + filename: "tools/widget.tsx", + report: () => {}, + get scopes(): never { + throw new Error("scopes should stay lazy"); + }, + get cfg(): never { + throw new Error("cfg should stay lazy"); + }, + }); + + expect(didCreateVisitors).toBe(false); + expect(visitors).toEqual({}); + }); + it("keeps capability-gated rules compatible when capabilities are unspecified", () => { const rule = defineRule({ id: "compiler-disabled-rule", diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts index e5eacfe766..aed63b8828 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts @@ -4,6 +4,7 @@ import { collectJsxRuntimeImports, jsxAttributeIsNonReactDialectMarker, } from "./non-react-jsx-dialect.js"; +import { isTestNoiseFilename } from "./is-testlike-filename.js"; import { skipNonProductionFiles } from "./skip-non-production-files.js"; import { shouldCreateRuleVisitors } from "./should-create-rule-visitors.js"; import type { Rule } from "./rule.js"; @@ -121,7 +122,7 @@ export const defineRule = (rule: RuleDefinition): Rule => { wrappedCreate = wrapCreateForReactJsxOnly(wrappedCreate as never) as never; } if (honorsTestNoise) { - wrappedCreate = skipNonProductionFiles(wrappedCreate); + wrappedCreate = skipNonProductionFiles(wrappedCreate, isTestNoiseFilename); } if (rule.disabledWhen) { wrappedCreate = wrapCreateForCapabilities(wrappedCreate, rule.disabledWhen); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.test.ts index 54d1e21bca..de5ec742f5 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vite-plus/test"; -import { isTestlikeFilename } from "./is-testlike-filename.js"; +import { isTestlikeFilename, isTestNoiseFilename } from "./is-testlike-filename.js"; describe("isTestlikeFilename", () => { it.each(["/workspace/test-stubs/trycompai-ui.tsx", "/workspace/src/test-stubs/trycompai-ui.tsx"])( @@ -13,3 +13,33 @@ describe("isTestlikeFilename", () => { expect(isTestlikeFilename("/workspace/src/components/dialog-stub.tsx")).toBe(false); }); }); + +describe("isTestNoiseFilename", () => { + it.each([ + "/workspace/src/components/tools/widget.tsx", + "src/demo/widget.tsx", + "app/migrations/page.tsx", + "components/spec/widget.tsx", + "src/perf/metrics.tsx", + "C:\\workspace\\src\\components\\tools\\widget.tsx", + ])("treats an ambiguous directory below a source root as production at %s", (filename) => { + expect(isTestNoiseFilename(filename)).toBe(false); + }); + + it.each([ + "/workspace/tools/widget.tsx", + "/workspace/examples/widget.tsx", + "/workspace/migrations/widget.tsx", + ])("keeps a root-level non-application directory testlike at %s", (filename) => { + expect(isTestNoiseFilename(filename)).toBe(true); + }); + + it.each([ + "/workspace/src/__tests__/tools/widget.tsx", + "/workspace/src/fixtures/demo/widget.tsx", + "/workspace/src/components/tools/widget.test.tsx", + "/workspace/src/.storybook/components/widget.tsx", + ])("keeps an explicit test surface testlike at %s", (filename) => { + expect(isTestNoiseFilename(filename)).toBe(true); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.ts index 419533f5f0..eb2ead2372 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-testlike-filename.ts @@ -1,26 +1,6 @@ -// Directory names that mark a file as part of a test / fixture / -// Storybook / Cypress / docs-site (`.dumi`) / example surface, regardless -// of the file's own suffix. -const NON_PRODUCTION_PATH_SEGMENTS: ReadonlyArray = [ - "/test/", - "/tests/", - "/testing/", - "/__tests__/", - "/__test__/", - "/__fixtures__/", - "/fixtures/", - "/__mocks__/", - "/mocks/", - "/testUtils/", - "/test-utils/", - "/test-stubs/", - "/testutils/", - "/cypress/", - "/playwright/", - "/.storybook/", - "/.dumi/", - "/stories/", - "/__stories__/", +// These names describe non-application code at the repository root, but they +// also name common product areas below an application source root. +const AMBIGUOUS_NON_PRODUCTION_PATH_SEGMENTS: ReadonlySet = new Set([ "/playground/", "/playgrounds/", "/examples/", @@ -29,23 +9,11 @@ const NON_PRODUCTION_PATH_SEGMENTS: ReadonlyArray = [ "/demos/", "/sandbox/", "/sandboxes/", - "/e2e/", - "/e2e-tests/", "/specs/", "/spec/", - "/integration-tests/", "/integration/", "/it/", - "/benchmarks/", - "/benchmark/", - "/__benchmarks__/", "/perf/", - "/perf-tests/", - // CLI / one-shot / build-time tooling — never shipped in the - // user-facing bundle, no render-perf or React-rule concerns. Captures - // top-level `scripts/`, `cli/`, `bin/`, `tooling/`, `tools/`, - // `codemods/`, `migrations/`, `generators/`, `runbooks/`, etc. as well - // as `src/scripts/...` shaped layouts. "/scripts/", "/cli/", "/bin/", @@ -63,6 +31,40 @@ const NON_PRODUCTION_PATH_SEGMENTS: ReadonlyArray = [ "/seeds/", "/seed/", "/dev-seeder/", +]); +const EMPTY_IGNORED_PATH_SEGMENTS: ReadonlySet = new Set(); + +// Directory names that mark a file as part of a test / fixture / +// Storybook / Cypress / docs-site (`.dumi`) / example surface, regardless +// of the file's own suffix. +const NON_PRODUCTION_PATH_SEGMENTS: ReadonlyArray = [ + "/test/", + "/tests/", + "/testing/", + "/__tests__/", + "/__test__/", + "/__fixtures__/", + "/fixtures/", + "/__mocks__/", + "/mocks/", + "/testUtils/", + "/test-utils/", + "/test-stubs/", + "/testutils/", + "/cypress/", + "/playwright/", + "/.storybook/", + "/.dumi/", + "/stories/", + "/__stories__/", + ...AMBIGUOUS_NON_PRODUCTION_PATH_SEGMENTS, + "/e2e/", + "/e2e-tests/", + "/integration-tests/", + "/benchmarks/", + "/benchmark/", + "/__benchmarks__/", + "/perf-tests/", ]; // True iff `filename` looks like test / spec / Storybook / Cypress / @@ -217,6 +219,8 @@ const sliceBelowSourceRoot = (filename: string): string => { // call per file. let lastFilename: string | undefined; let lastResult = false; +let lastTestNoiseFilename: string | undefined; +let lastTestNoiseResult = false; export const isTestlikeFilename = (rawFilename: string | undefined): boolean => { if (!rawFilename) return false; @@ -226,6 +230,22 @@ export const isTestlikeFilename = (rawFilename: string | undefined): boolean => return lastResult; }; +export const isTestNoiseFilename = (rawFilename: string | undefined): boolean => { + if (!rawFilename) return false; + if (rawFilename === lastTestNoiseFilename) return lastTestNoiseResult; + lastTestNoiseFilename = rawFilename; + const filename = rawFilename.replaceAll("\\", "/"); + const rootedFilename = filename.startsWith("/") ? filename : `/${filename}`; + const isBelowSourceRoot = SOURCE_ROOT_SEGMENTS.some((segment) => + rootedFilename.includes(segment), + ); + lastTestNoiseResult = computeIsTestlikeFilename( + rootedFilename, + isBelowSourceRoot ? AMBIGUOUS_NON_PRODUCTION_PATH_SEGMENTS : EMPTY_IGNORED_PATH_SEGMENTS, + ); + return lastTestNoiseResult; +}; + export const isTestlikeFilenameIgnoringPathSegments = ( rawFilename: string | undefined, ignoredPathSegments: ReadonlySet, @@ -233,7 +253,7 @@ export const isTestlikeFilenameIgnoringPathSegments = ( const computeIsTestlikeFilename = ( rawFilename: string, - ignoredPathSegments: ReadonlySet = new Set(), + ignoredPathSegments: ReadonlySet = EMPTY_IGNORED_PATH_SEGMENTS, ): boolean => { const filename = rawFilename.replaceAll("\\", "/"); const lastSlash = filename.lastIndexOf("/"); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts index ea062481d2..4eb7a43948 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts @@ -10,6 +10,6 @@ import type { RuleVisitors } from "./rule-visitors.js"; // `new Function` / a token in web storage is not a real vulnerability in test // scaffolding that never reaches a browser. export const skipNonProductionFiles = - (create: (context: RuleContext) => RuleVisitors) => + (create: (context: RuleContext) => RuleVisitors, isNonProductionFilename = isTestlikeFilename) => (context: RuleContext): RuleVisitors => - isTestlikeFilename(context.filename) ? EMPTY_RULE_VISITORS : create(context); + isNonProductionFilename(context.filename) ? EMPTY_RULE_VISITORS : create(context); diff --git a/packages/react-doctor/tests/regressions/rn-and-motion.test.ts b/packages/react-doctor/tests/regressions/rn-and-motion.test.ts index 068d574904..e002e977a7 100644 --- a/packages/react-doctor/tests/regressions/rn-and-motion.test.ts +++ b/packages/react-doctor/tests/regressions/rn-and-motion.test.ts @@ -10,6 +10,7 @@ * children * #581 — fbtee `` / `` translation tags stay transparent to * the `` boundary (so raw text inside them isn't flagged) + * #1722 — `` content is safe inside verified text wrappers * #76 — maintained Expo packages are not treated as legacy packages * #94 — `MotionConfig reducedMotion="user"` must satisfy the * reduced-motion accessibility check (so the rule doesn't @@ -276,7 +277,9 @@ describe("rn-no-raw-text resolves imported components across files", () => { `export const App = () => (\n` + ` <>\n` + ` Safe label\n` + + ` Safe translated label\n` + ` Crashing text\n` + + ` Crashing translated text\n` + ` \n` + `);\n`, }, @@ -293,9 +296,15 @@ describe("rn-no-raw-text resolves imported components across files", () => { .filter((diagnostic) => diagnostic.rule === "rn-no-raw-text") .map((diagnostic) => diagnostic.message); - expect(rnRawTextMessages).toHaveLength(1); - expect(rnRawTextMessages[0]).toContain("Crashing text"); + expect(rnRawTextMessages).toHaveLength(2); + expect(rnRawTextMessages.some((message) => message.includes("Crashing text"))).toBe(true); + expect(rnRawTextMessages.some((message) => message.includes("Crashing translated text"))).toBe( + true, + ); expect(rnRawTextMessages.some((message) => message.includes("Safe label"))).toBe(false); + expect(rnRawTextMessages.some((message) => message.includes("Safe translated label"))).toBe( + false, + ); }); it("follows a wrapper that forwards children through another component in the same module", async () => { @@ -399,7 +408,7 @@ export const App = () => ( expect(diagnostics).toHaveLength(0); }); - it("still reports raw text when is outside ", async () => { + it("does not report when a component returns only fbt (issue #1729)", async () => { const projectDirectory = buildFbteeProject( "issue-581-fbt-outside-text", `export const App = () => Welcome; @@ -407,7 +416,7 @@ export const App = () => ( ); const diagnostics = await getRnNoRawTextDiagnostics(projectDirectory); - expect(diagnostics).toHaveLength(1); + expect(diagnostics).toHaveLength(0); }); });