fix(react-doctor): forward reactMajorVersion in programmatic diagnose() + cleanups#174
Merged
fix(react-doctor): forward reactMajorVersion in programmatic diagnose() + cleanups#174
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…() + cleanups H1 (regression introduced by #172) — `diagnose()` in `src/index.ts` forgot to forward `reactMajorVersion` to `runOxlint`. After the directional version-gating change in #172, that meant every "prefer-newer-api" rule (today: `prefer-use-effect-event`) was silently skipped for every programmatic API consumer, even on React 19+ projects. The CLI (`scan.ts`) was unaffected because it always passed the version explicitly. Fix is one line + one import — mirror what `scan.ts` already does. Added `tests/diagnose.test.ts` with a regression test that asserts `prefer-use-effect-event` fires on a React 19 fixture, plus a symmetric guard that it stays skipped when the React version can't be parsed (e.g. a github: range). H2 — updated the stale docstring on `runOxlint`'s `reactMajorVersion` field. The doc still claimed "`null` means unknown — leave those rules enabled" but after #172 the null branch is directional (deprecation-warning rules stay on, prefer-newer-api rules go off). M1 — `SUB_HANDLER_DIRECT_CALLEE_NAMES` was just an alias of `TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES`. Both names existed for narrative reasons in different files; knip flagged the duplicate export. Collapsed to the canonical name and updated the one consumer (`isCallExpressionWithSubHandlerCallee` in state-and-effects). L1 — moved `walkInsideStatementBlocks` from inline in `state-and-effects.ts` to `plugin/helpers.ts` next to its sibling `walkAst`. It already had four call sites and is the natural "synchronous-only" walker for any rule asking what runs inside an effect's own body — colocating with `walkAst` makes future rules discover it. L2 (audit finding) — proposed receiver-gating `post`/`put`/`patch` in `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES` INTENTIONALLY NOT TAKEN. The canonical "You Might Not Need an Effect" §6 example is `post(jsonToSubmit)` as a bare callee, so removing those names breaks textbook detection (3 existing tests / fixtures). Documented the trade-off in the constants.ts docstring. Validation - 642 tests passing (640 baseline + 2 new diagnose-API regressions) - typecheck / lint / format clean Co-authored-by: Cursor <cursoragent@cursor.com>
Pre-existing formatting issue introduced in 6afdc04 — the script was committed unformatted, breaking `pnpm format:check` on every PR branched off main since. Picked up by rebasing this PR. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit pass on
mainafter #170/#171/#172. One real regression introduced by #172, plus a stale doc, a duplicate-export alias, and a code-org cleanup.H1 —
prefer-use-effect-eventsilently disabled in the programmatic API (regression from #172)diagnose()inpackages/react-doctor/src/index.tsdid not forwardreactMajorVersiontorunOxlint. After the directional version-gating change in #172, every "prefer-newer-api" rule (today:prefer-use-effect-event) was silently skipped for every programmatic API consumer (e.g.react-doctor/api), even on React 19+ projects. The CLI (scan.ts:589) was unaffected because it always passed the version explicitly.Fix is one line + one import — mirror what
scan.tsalready does.Added
tests/diagnose.test.tswith:prefer-use-effect-eventfires on a React 19 fixture viadiagnose()github:range), proving the version-gate boundary is honoredH2 — stale docstring on
runOxlint'sreactMajorVersionThe doc still claimed "
nullmeans unknown — leave those rules enabled" but after #172 the null branch is directional (deprecation-warning rules stay on, prefer-newer-api rules go off). Updated to reflect the actual semantics.M1 — duplicate alias
SUB_HANDLER_DIRECT_CALLEE_NAMESSUB_HANDLER_DIRECT_CALLEE_NAMESwas just= TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES. Two names existed in different files for narrative reasons; knip flagged it. Collapsed to the canonical name and updated the one consumer (isCallExpressionWithSubHandlerCallee).L1 — moved
walkInsideStatementBlockstoplugin/helpers.tsAlready had four call sites in
state-and-effects.ts. Colocated with its siblingwalkAstso future rules discover it.L2 (audit finding) — INTENTIONALLY NOT TAKEN
I proposed receiver-gating
post/put/patchinEVENT_TRIGGERED_SIDE_EFFECT_CALLEES. The canonical "You Might Not Need an Effect" §6 example ispost(jsonToSubmit)as a bare callee, so removing those names breaks textbook detection (3 existing tests / fixtures fail). Documented the trade-off in the constants.ts docstring instead.Test plan
pnpm typecheck— cleanpnpm lint— 0 warnings, 0 errorspnpm format:check— cleanpnpm test— 642 passing (640 baseline + 2 newdiagnose()API regressions)Made with Cursor
Note
Low Risk
Low risk: mostly wiring/cleanup changes plus new regression tests; behavior change is limited to version-gating which only affects which lint rules fire for programmatic
diagnose()consumers.Overview
Fixes a regression where programmatic
diagnose()scans didn’t pass the detected React major version intorunOxlint, causing React-version-gated “prefer-newer-api” rules (e.g.prefer-use-effect-event) to be skipped even on React 19+ projects.Adds regression tests covering the React 19 firing case and the “unknown/unparseable version” skip case, updates the
reactMajorVersionoption docs to reflect directional gating semantics, and does small internal cleanups (dedupe timer/scheduler callee naming, movewalkInsideStatementBlocksinto shared helpers, and minor script formatting).Reviewed by Cursor Bugbot for commit 95ae940. Bugbot is set up for automated code reviews on this repo. Configure here.