diff --git a/packages/react-doctor/src/index.ts b/packages/react-doctor/src/index.ts index b0b9084..b2e9b3d 100644 --- a/packages/react-doctor/src/index.ts +++ b/packages/react-doctor/src/index.ts @@ -24,6 +24,7 @@ import { clearProjectCache, discoverProject } from "./utils/discover-project.js" import { computeJsxIncludePaths } from "./utils/jsx-include-paths.js"; import { clearConfigCache, loadConfig } from "./utils/load-config.js"; import { mergeAndFilterDiagnostics } from "./utils/merge-and-filter-diagnostics.js"; +import { parseReactMajor } from "./utils/parse-react-major.js"; import { clearPackageJsonCache } from "./utils/read-package-json.js"; import { createNodeReadFileLinesSync } from "./utils/read-file-lines-node.js"; import { resolveLintIncludePaths } from "./utils/resolve-lint-include-paths.js"; @@ -130,6 +131,7 @@ export const diagnose = async ( framework: projectInfo.framework, hasReactCompiler: projectInfo.hasReactCompiler, hasTanStackQuery: projectInfo.hasTanStackQuery, + reactMajorVersion: parseReactMajor(projectInfo.reactVersion), includePaths: lintIncludePaths, customRulesOnly: userConfig?.customRulesOnly ?? false, respectInlineDisables: effectiveRespectInlineDisables, diff --git a/packages/react-doctor/src/plugin/constants.ts b/packages/react-doctor/src/plugin/constants.ts index 3ebd3b1..bf1f0f0 100644 --- a/packages/react-doctor/src/plugin/constants.ts +++ b/packages/react-doctor/src/plugin/constants.ts @@ -417,8 +417,6 @@ export const TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES = new Set([ "queueMicrotask", ]); -export const SUB_HANDLER_DIRECT_CALLEE_NAMES = TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES; - // Timer registrations that ALWAYS need a corresponding cleanup call // (a stricter subset of the scheduler list above — `requestAnimationFrame` // and friends already invoke once and self-clean, but `setTimeout` / @@ -575,6 +573,12 @@ export const EXTERNAL_SYNC_OBSERVER_CONSTRUCTORS = new Set([ // network/analytics side effects. Detection still works via the // receiver-bound member-call shape (`analytics.track(...)`, // `api.del(...)`) in `EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS`. +// +// `post` / `put` / `patch` are KEPT here — the canonical "You Might +// Not Need an Effect" §6 example is `post(jsonToSubmit)` as a bare +// callee, so removing them would silently miss the textbook case. +// The trade-off (FPs on user helpers named `post(...)`) is acceptable +// at this scope. export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([ ...FETCH_CALLEE_NAMES, // Network shorthand verbs (article uses `post`) diff --git a/packages/react-doctor/src/plugin/helpers.ts b/packages/react-doctor/src/plugin/helpers.ts index a5e83af..8293833 100644 --- a/packages/react-doctor/src/plugin/helpers.ts +++ b/packages/react-doctor/src/plugin/helpers.ts @@ -53,6 +53,41 @@ export const walkAst = (node: EsTreeNode, visitor: (child: EsTreeNode) => boolea } }; +// HACK: variant of `walkAst` that descends through control-flow blocks +// (IfStatement / TryStatement / SwitchCase / loops / labels) but stops +// at any nested function boundary. Used by rules that ask "what runs +// SYNCHRONOUSLY inside this effect's body?" — counts the +// `if (cond) setX(...)` write but ignores the deferred +// `setTimeout(() => setX(...))` one. +// +// Unlike `walkAst`, this one does not support pruning via `false` +// return — descent is always complete except at function boundaries. +export const walkInsideStatementBlocks = ( + node: EsTreeNode, + visitor: (child: EsTreeNode) => void, +): void => { + if (!node || typeof node !== "object") return; + if ( + node.type === "FunctionDeclaration" || + node.type === "FunctionExpression" || + node.type === "ArrowFunctionExpression" + ) { + return; + } + visitor(node); + for (const key of Object.keys(node)) { + if (key === "parent") continue; + const child = node[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === "object" && item.type) walkInsideStatementBlocks(item, visitor); + } + } else if (child && typeof child === "object" && child.type) { + walkInsideStatementBlocks(child, visitor); + } + } +}; + export const isSetterIdentifier = (name: string): boolean => SETTER_PATTERN.test(name); export const isSetterCall = (node: EsTreeNode): boolean => diff --git a/packages/react-doctor/src/plugin/rules/state-and-effects.ts b/packages/react-doctor/src/plugin/rules/state-and-effects.ts index 8a96ee3..29ca78e 100644 --- a/packages/react-doctor/src/plugin/rules/state-and-effects.ts +++ b/packages/react-doctor/src/plugin/rules/state-and-effects.ts @@ -17,8 +17,8 @@ import { NAVIGATION_RECEIVER_NAMES, REACT_HANDLER_PROP_PATTERN, RELATED_USE_STATE_THRESHOLD, - SUB_HANDLER_DIRECT_CALLEE_NAMES, SUBSCRIPTION_METHOD_NAMES, + TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES, TIMER_CALLEE_NAMES_REQUIRING_CLEANUP, TIMER_CLEANUP_CALLEE_NAMES, TRIVIAL_DERIVATION_CALLEE_NAMES, @@ -41,6 +41,7 @@ import { isSetterIdentifier, isUppercaseName, walkAst, + walkInsideStatementBlocks, } from "../helpers.js"; import type { EsTreeNode, Rule, RuleContext } from "../types.js"; @@ -688,32 +689,6 @@ const collectReturnExpressions = (componentBody: EsTreeNode): EsTreeNode[] => { return returns; }; -const walkInsideStatementBlocks = ( - node: EsTreeNode, - visitor: (child: EsTreeNode) => void, -): void => { - if (!node || typeof node !== "object") return; - if ( - node.type === "FunctionDeclaration" || - node.type === "FunctionExpression" || - node.type === "ArrowFunctionExpression" - ) { - return; - } - visitor(node); - for (const key of Object.keys(node)) { - if (key === "parent") continue; - const child = node[key]; - if (Array.isArray(child)) { - for (const item of child) { - if (item && typeof item === "object" && item.type) walkInsideStatementBlocks(item, visitor); - } - } else if (child && typeof child === "object" && child.type) { - walkInsideStatementBlocks(child, visitor); - } - } -}; - const collectIdentifierNames = (expression: EsTreeNode): Set => { const names = new Set(); walkAst(expression, (child: EsTreeNode) => { @@ -2353,9 +2328,9 @@ export const noMutableInDeps: Rule = { // - a destructured prop named `on[A-Z]…`, OR // - a local declared via `const F = useCallback(...)` // (3) every read of `F` inside the effect body sits inside a sub- -// handler (SUB_HANDLER_DIRECT_CALLEE_NAMES, OR a MemberExpression -// whose property is in SUBSCRIPTION_METHOD_NAMES — same set the -// prefer-use-sync-external-store family uses) +// handler (TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES, OR a +// MemberExpression whose property is in SUBSCRIPTION_METHOD_NAMES +// — same set the prefer-use-sync-external-store family uses) // (4) `F` is NEVER read at the effect's own top level const collectFunctionTypedLocalBindings = (componentBody: EsTreeNode): Set => { const functionTypedLocals = new Set(); @@ -2393,7 +2368,7 @@ const findEnclosingFunctionInsideEffect = ( const isCallExpressionWithSubHandlerCallee = (callExpression: EsTreeNode): boolean => { if (callExpression?.type !== "CallExpression") return false; const callee = callExpression.callee; - if (callee?.type === "Identifier" && SUB_HANDLER_DIRECT_CALLEE_NAMES.has(callee.name)) { + if (callee?.type === "Identifier" && TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES.has(callee.name)) { return true; } if ( diff --git a/packages/react-doctor/src/utils/run-oxlint.ts b/packages/react-doctor/src/utils/run-oxlint.ts index d13b752..246255c 100644 --- a/packages/react-doctor/src/utils/run-oxlint.ts +++ b/packages/react-doctor/src/utils/run-oxlint.ts @@ -848,9 +848,13 @@ interface RunOxlintOptions { hasTanStackQuery: boolean; /** * Major version of React detected for the project. Forwarded to - * `createOxlintConfig` so React-19-deprecation rules only fire on - * projects where they actually apply. `null` means "unknown — leave - * those rules enabled". + * `createOxlintConfig`, which gates rules directionally: + * - `"deprecation-warning"` rules (e.g. `no-default-props`) stay + * enabled when this is `null` so mid-migration projects still + * get the warning even if version detection failed. + * - `"prefer-newer-api"` rules (e.g. `prefer-use-effect-event`) are + * skipped when this is `null` to avoid recommending APIs that + * may not exist in the consumer's React version. */ reactMajorVersion?: number | null; includePaths?: string[]; diff --git a/packages/react-doctor/tests/diagnose.test.ts b/packages/react-doctor/tests/diagnose.test.ts new file mode 100644 index 0000000..fc3b6ce --- /dev/null +++ b/packages/react-doctor/tests/diagnose.test.ts @@ -0,0 +1,77 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterAll, describe, expect, it } from "vite-plus/test"; + +import { diagnose } from "../src/index.js"; +import { setupReactProject } from "./regressions/_helpers.js"; + +const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "rd-diagnose-api-")); + +afterAll(() => { + fs.rmSync(tempRoot, { recursive: true, force: true }); +}); + +describe("diagnose() programmatic API", () => { + // Regression: pre-fix the programmatic `diagnose()` entry forgot to + // forward `reactMajorVersion` to `runOxlint`. After the directional + // version-gating change, that meant every "prefer-newer-api" rule + // (today: `prefer-use-effect-event`) was silently skipped for all + // programmatic API consumers, even on React 19+ projects. The CLI + // entry (`scan.ts`) was unaffected because it always passed the + // version explicitly. + it("emits prefer-use-effect-event diagnostics on a React 19 project (the prefer-newer-api version-gated rule fires)", async () => { + const projectDir = setupReactProject(tempRoot, "diagnose-prefer-use-effect-event-fires", { + files: { + "src/Debounced.tsx": `import { useEffect, useState } from "react"; + +export const Debounced = ({ onChange }: { onChange: (value: string) => void }) => { + const [text, setText] = useState(""); + useEffect(() => { + const id = setTimeout(() => onChange(text), 300); + return () => clearTimeout(id); + }, [text, onChange]); + return setText(event.target.value)} />; +}; +`, + }, + }); + + const result = await diagnose(projectDir, { lint: true, deadCode: false }); + const preferUseEffectEventHits = result.diagnostics.filter( + (diagnostic) => diagnostic.rule === "prefer-use-effect-event", + ); + expect(preferUseEffectEventHits.length).toBeGreaterThanOrEqual(1); + }); + + it("skips prefer-use-effect-event when the project's React version cannot be resolved (no react dep)", async () => { + // Symmetric guard: when the project has no React dependency the + // function throws before lint runs, so we synthesize a project + // with an unresolvable React version range. Its major can't be + // parsed, so `parseReactMajor` returns null and the prefer-newer- + // api rule should be skipped pessimistically — confirming the + // forward really is honoring the version-gate boundary. + const projectDir = setupReactProject(tempRoot, "diagnose-prefer-use-effect-event-skipped", { + reactVersion: "github:facebook/react", + files: { + "src/Debounced.tsx": `import { useEffect, useState } from "react"; + +export const Debounced = ({ onChange }: { onChange: (value: string) => void }) => { + const [text, setText] = useState(""); + useEffect(() => { + const id = setTimeout(() => onChange(text), 300); + return () => clearTimeout(id); + }, [text, onChange]); + return setText(event.target.value)} />; +}; +`, + }, + }); + + const result = await diagnose(projectDir, { lint: true, deadCode: false }); + const preferUseEffectEventHits = result.diagnostics.filter( + (diagnostic) => diagnostic.rule === "prefer-use-effect-event", + ); + expect(preferUseEffectEventHits).toHaveLength(0); + }); +}); diff --git a/scripts/benchmark-scores.ts b/scripts/benchmark-scores.ts index 0e7dcbd..341791e 100644 --- a/scripts/benchmark-scores.ts +++ b/scripts/benchmark-scores.ts @@ -35,8 +35,7 @@ interface BenchmarkResult { errorMessage: string | null; } -const gh = (args: string): string => - execSync(`gh ${args}`, { encoding: "utf-8" }).trim(); +const gh = (args: string): string => execSync(`gh ${args}`, { encoding: "utf-8" }).trim(); const getLatestRunId = (): number => { const output = gh( @@ -157,9 +156,7 @@ const main = async (): Promise => { const results = await Promise.all( artifacts.map(({ id }) => downloadAndParseArtifact(id, tempDirectory)), ); - const validResults = results.filter( - (result): result is BenchmarkResult => result !== null, - ); + const validResults = results.filter((result): result is BenchmarkResult => result !== null); if (validResults.length === 0) { console.error("No valid benchmark results found");