feat(linter): react/exhaustive-deps by camc314 · Pull Request #7151 · oxc-project/oxc

camc314 · 2024-11-06T00:22:45Z

Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 !

closes #2637
relates to #2174

graphite-app · 2024-11-06T00:22:49Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

crates/oxc_linter/src/rules/react_hooks/exhaustive_deps.rs

camc314 · 2024-11-07T01:32:14Z

Rough first draft but most of the way their. would appreciate some early reviews as this was a massive amount of work and i don't want to go too far 🙂

Boshen · 2024-11-08T01:20:22Z

Rough first draft but most of the way their. would appreciate some early reviews as this was a massive amount of work and i don't want to go too far 🙂

This is amazing! I would run it through oxlint ecosystem ci and see what happens.

camchenry

This looks pretty good overall, I didn't thoroughly review all of the code (there is a lot!) but I took a look at the diagnostics and have a few help text suggestions. I think keeping this in nursery and continuing to iterate on it would be good

crates/oxc_linter/src/rules/react_hooks/exhaustive_deps.rs

Boshen · 2024-11-09T05:16:33Z

Note: once merged, manually add this rule to oxlint-ecosystem-ci's CI script. We then change this to correctness after running it for 2 - 4 weeks.

camchenry

it's hard to review all of this code, but I have skimmed over most of it and not too much stuck out to me. I think this is ready to start being tested in nursery

crates/oxc_linter/src/service/runtime.rs

camc314 · 2024-11-09T15:51:25Z

@Boshen did it look ok in the ecosystem ci? https://github.com/oxc-project/oxlint-ecosystem-ci/actions/runs/11755920122/job/32751524336

thanks for testing for me 🙂

codspeed-hq · 2024-11-09T15:56:03Z

CodSpeed Performance Report

Merging #7151 will not alter performance

_{Comparing c/exhaustive-deps2 (3dcac1a) with main (c5f4ee7)}

Summary

✅ 30 untouched benchmarks

crates/oxc_linter/src/rules/react_hooks/exhaustive_deps.rs

Boshen · 2024-11-11T08:36:33Z

Merge activity

Nov 11, 3:36 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Nov 11, 3:36 AM EST: A user added this pull request to the Graphite merge queue.
Nov 11, 3:43 AM EST: The Graphite merge queue couldn't merge this PR because it was in draft mode.
Nov 12, 6:37 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Nov 12, 6:42 AM EST: A user added this pull request to the Graphite merge queue.
Nov 12, 6:47 AM EST: A user merged this pull request with the Graphite merge queue.

Boshen · 2024-11-11T08:43:21Z

Hit the merge button too quickly 😅

In oxlint-ecosystem-ci/repos:

~/oxc/target/release/oxlint -A all -W exhaustive-deps
Finished in 5.5s on 117561 files with 1 rules using 8 threads.
Found 536 warnings and 23 errors.

Some false positives:

affine  canary ❯ ~/oxc/oxc/target/release/oxlint -A all -W exhaustive-deps

  ⚠ react_hooks(exhaustive-deps): React Hook useMemo has a missing dependency: 'T'
     ╭─[packages/frontend/core/src/components/page-list/virtualized-list.tsx:142:6]
 141 │     return items;
 142 │   }, [groupCollapsedState, groups]);
     ·      ─────────────────────────────
 143 │ };
     ╰────
  help: Either include it or remove the dependency array.

It's picking up type annotation

    const items: VirtuosoItem<T>[] = [];

Boshen · 2024-11-11T08:43:59Z

Do you also need to include function calls?

  ⚠ react_hooks(exhaustive-deps): React Hook useEffect has a missing dependency: 'makeMappingRequest'
    ╭─[kibana/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/index_error.tsx:83:6]
 82 │     makeMappingRequest({ indexName });
 83 │   }, [indexName]);
    ·      ───────────
 84 │
    ╰────
  help: Either include it or remove the dependency array.

Boshen · 2024-11-11T08:46:56Z

  ⚠ react_hooks(exhaustive-deps): React Hook useCallback has unnecessary dependency: dataVisualizerListState.sortField
     ╭─[kibana/x-pack/plugins/data_visualizer/public/application/index_data_visualizer/hooks/use_field_stats.ts:307:5]
 306 │     dataVisualizerListState.sortDirection,
 307 │     dataVisualizerListState.sortField,
     ·     ───────────────────────
 308 │     samplingProbability,
     ╰────
  help: Either include it or remove the dependency array.

The span is not pointing at the member expression.

Boshen · 2024-11-11T08:47:50Z

This looks pretty weird

  ⚠ react_hooks(exhaustive-deps): React Hook useEffect has missing dependencies: 'searchParams.get', 't', and 'form.setValue'
    ╭─[cal/packages/features/eventtypes/components/DuplicateDialog.tsx:67:6]
 66 │     }
 67 │   }, [searchParams?.get("dialog")]);
    ·      ─────────────────────────────
 68 │
    ╰────
  help: Either include it or remove the dependency array.

Boshen · 2024-11-11T08:49:12Z

kibana has the most violations, you may want to test it during developement.

camc314 · 2024-11-11T09:41:03Z

This looks pretty weird

  ⚠ react_hooks(exhaustive-deps): React Hook useEffect has missing dependencies: 'searchParams.get', 't', and 'form.setValue'
    ╭─[cal/packages/features/eventtypes/components/DuplicateDialog.tsx:67:6]
 66 │     }
 67 │   }, [searchParams?.get("dialog")]);
    ·      ─────────────────────────────
 68 │
    ╰────
  help: Either include it or remove the dependency array.
``

🙂 looks like we're following the same behaviour as eslint

crates/oxc_linter/src/rules/react_hooks/exhaustive_deps.rs

crates/oxc_linter/src/snapshots/exhaustive_deps.snap

camc314 · 2024-11-11T11:20:14Z

Do you also need to include function calls?

  ⚠ react_hooks(exhaustive-deps): React Hook useEffect has a missing dependency: 'makeMappingRequest'
    ╭─[kibana/x-pack/plugins/enterprise_search/public/applications/enterprise_search_content/components/search_index/index_error.tsx:83:6]
 82 │     makeMappingRequest({ indexName });
 83 │   }, [indexName]);
    ·      ───────────
 84 │
    ╰────
  help: Either include it or remove the dependency array.

looks like they've got it disabled? for this file https://github.com/elastic/kibana//blob/2466a172af66452bdd939dddc56506faba7ffb7a/.eslintrc.js#L1623

when i run eslint on this file, it reports (same as us)

camc314 · 2024-11-11T11:37:08Z

@Boshen after a4f7675 i don't there are more false positives in affine at least

Boshen · 2024-11-12T03:17:34Z

  ⚠ react_hooks(exhaustive-deps): React Hook useCallback received a function whose dependencies are unknown.
    ╭─[kibana/src/plugins/console/public/application/hooks/use_save_current_text_object.ts:27:5]
 26 │       return useCallback(
 27 │ ╭─▶     throttle(
 28 │ │         (text: string) => {
 29 │ │           const { current: promise } = promiseChainRef;
 30 │ │           if (!currentTextObject) return;
 31 │ │           promise.finally(() =>
 32 │ │             objectStorageClient.text.update({ ...currentTextObject, text, updatedAt: Date.now() })
 33 │ │           );
 34 │ │         },
 35 │ │         WAIT_MS,
 36 │ │         { trailing: true }
 37 │ ╰─▶     ),
 38 │         [objectStorageClient, currentTextObject]
    ╰────
  help: Pass an inline function instead.

Interesting case, how should this be handled?

Boshen

Feel free to merge after a final self review.

camc314 · 2024-11-12T08:54:39Z

  ⚠ react_hooks(exhaustive-deps): React Hook useCallback received a function whose dependencies are unknown.
    ╭─[kibana/src/plugins/console/public/application/hooks/use_save_current_text_object.ts:27:5]
 26 │       return useCallback(
 27 │ ╭─▶     throttle(
 28 │ │         (text: string) => {
 29 │ │           const { current: promise } = promiseChainRef;
 30 │ │           if (!currentTextObject) return;
 31 │ │           promise.finally(() =>
 32 │ │             objectStorageClient.text.update({ ...currentTextObject, text, updatedAt: Date.now() })
 33 │ │           );
 34 │ │         },
 35 │ │         WAIT_MS,
 36 │ │         { trailing: true }
 37 │ ╰─▶     ),
 38 │         [objectStorageClient, currentTextObject]
    ╰────
  help: Pass an inline function instead.

Interesting case, how should this be handled?

lol we do the same thing as react, only we're currently reporting on a different line so it wasn't being ignored

camc314 · 2024-11-12T08:56:24Z

@Boshen i guess we should be reporting the same (or at least very similar spans to react? if ok with you, i'll make an issue to track bits that aren't exactly complete for this rule

Boshen · 2024-11-12T11:36:59Z

@Boshen i guess we should be reporting the same (or at least very similar spans to react? if ok with you, i'll make an issue to track bits that aren't exactly complete for this rule

Indeed, current span breaks already written eslint ignores.

@alisnic

Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 ! still a draft. current state: 3x false positives (all todo with refs) 3x false negatives (TS will catch these 13x false negatvies todo with refs 1x false negative TODO closes #2637 relates to #2174

@alisnic

Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 ! still a draft. current state: 3x false positives (all todo with refs) 3x false negatives (TS will catch these 13x false negatvies todo with refs 1x false negative TODO closes #2637 relates to #2174

@alisnic

Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 ! still a draft. current state: 3x false positives (all todo with refs) 3x false negatives (TS will catch these 13x false negatvies todo with refs 1x false negative TODO closes #2637 relates to #2174

@alisnic

Firstly, a massive thanks to @alisnic for starting this (incredibly complicated) lint rule in #2637 ! still a draft. current state: 3x false positives (all todo with refs) 3x false negatives (TS will catch these 13x false negatvies todo with refs 1x false negative TODO closes #2637 relates to #2174

## [0.12.0] - 2024-11-20 - 20d9080 linter: [**BREAKING**] Override plugins array when passed in config file (#7303) (camchenry) ### Features - 1d9f528 linter: Implement `unicorn/prefer-string-raw` lint rule (#7335) (Ryan Walker) - d445e0f linter: Implement `unicorn/consistent-existence-index-check` (#7262) (Ryan Walker) - 01ddf37 linter: Add `allowReject` option to `no-useless-promise-resolve-reject` (#7274) (no-yan) - 755a31b linter: Support bind function case for compatibility with `promise/no-return-wrap` (#7232) (no-yan) - 428770e linter: Add `import/no-namespace` rule (#7229) (Dmitry Zakharov) - 9c91151 linter: Implement typescript/no-empty-object-type (#6977) (Orenbek) - 2268a0e linter: Support `overrides` config field (#6974) (DonIsaac) - 3dcac1a linter: React/exhaustive-deps (#7151) (camc314) - d3a0119 oxlint: Add `cwd` property to `LintRunner` (#7352) (Alexander S.) ### Bug Fixes - ba0b2ff editor: Reload workspace configuration after change (#7302) (Alexander S.) - bc0e72c linter: Handle user variables correctly for import/no_commonjs (#7316) (Dmitry Zakharov) - bf839c1 linter: False positive in `jest/expect-expect` (#7341) (dalaoshu) - ff2a1d4 linter: Move `exhaustive-deps` to `react` (#7251) (camc314) - df5c535 linter: Revert unmatched rule error (#7257) (Cameron A McHenry) - c4ed230 linter: Fix false positive in eslint/no-cond-assign (#7241) (camc314) - ef847da linter: False positive in `jsx-a11y/iframe-has-title` (#7253) (dalaoshu) - 62b6327 linter: React/exhaustive-deps update span for unknown deps diagnostic (#7249) (camc314) ### Documentation - 4c124a8 editor/vscode: Update VS Code readme with installation instructions and available features (#7306) (Nicholas Rayburn) ### Refactor - c6a4868 linter: Temporarily remove unknown rules checking (#7260) (camchenry) ### Testing - 5190b7f editor: Add test setup (#7361) (Alexander S.) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>

…dentifier it is a dependency of hook (#18350) From what I understand from the logic, only variables declared at the component level should be dependencies; otherwise, they are normal variables outside of the component or inside the child scope. import { useEffect } from "react"; 1. Variable was declared outside the component scope ```tsx const id = crypto.randomUUID(); function MyComponent() { useEffect(() => { console.log(id); }, []); return <div />; } ``` 2. Variable was declared inside a child scope ```tsx function MyComponent() { useEffect(() => { const id = crypto.randomUUID(); console.log(id); }, []); return <div />; } ``` 3. Variable was redeclared in the same scope (should add `id` to dependencies) ```tsx function MyComponent() { const id = crypto.randomUUID(); useEffect(() => { const id = crypto.randomUUID(); console.log(id); }, []); return <div />; } ``` Only case 3) should add `id` into the dependencies. So simplifying the logic to check `declaration.scope_id() != component_scope_id` is sufficient. ### Performance This should have a significant performance improvement as it avoids the unnecessary two loops. By checking the initial PR of this rule #7151, there are also no performance changes, so I guess there are no such cases in the benchmark files.

github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Nov 6, 2024

camc314 force-pushed the c/exhaustive-deps2 branch 3 times, most recently from 95fa95d to e4715ae Compare November 6, 2024 21:41

camc314 commented Nov 6, 2024

View reviewed changes

crates/oxc_linter/src/rules/react_hooks/exhaustive_deps.rs Outdated Show resolved Hide resolved

camc314 force-pushed the c/exhaustive-deps2 branch 2 times, most recently from a9758db to 1cd2f0b Compare November 6, 2024 22:45

camc314 changed the title ~~DRAFT: feat(linter): react/exhaustive-deps~~ feat(linter): react/exhaustive-deps Nov 6, 2024

github-actions bot added the C-enhancement Category - New feature or request label Nov 6, 2024

camc314 force-pushed the c/exhaustive-deps2 branch 2 times, most recently from 99854fb to 97c5918 Compare November 6, 2024 23:46

Boshen mentioned this pull request Nov 7, 2024

WIP: react-hooks/exhaustive-deps #2637

Closed

camc314 marked this pull request as ready for review November 7, 2024 17:57

camchenry reviewed Nov 8, 2024

View reviewed changes

camchenry approved these changes Nov 9, 2024

View reviewed changes

crates/oxc_linter/src/service/runtime.rs Outdated Show resolved Hide resolved

camc314 force-pushed the c/exhaustive-deps2 branch from d66e466 to 8f927a2 Compare November 9, 2024 15:36

camc314 commented Nov 9, 2024

View reviewed changes

crates/oxc_linter/src/rules/react_hooks/exhaustive_deps.rs Outdated Show resolved Hide resolved

Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 11, 2024

Boshen marked this pull request as draft November 11, 2024 08:39

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Nov 11, 2024

Boshen marked this pull request as ready for review November 11, 2024 08:47

camc314 commented Nov 11, 2024

View reviewed changes

crates/oxc_linter/src/rules/react_hooks/exhaustive_deps.rs Outdated Show resolved Hide resolved

camc314 commented Nov 11, 2024

View reviewed changes

crates/oxc_linter/src/snapshots/exhaustive_deps.snap Show resolved Hide resolved

Boshen approved these changes Nov 12, 2024

View reviewed changes

camc314 mentioned this pull request Nov 12, 2024

linter: followups for react/exhaustive-dep #7246

Closed

Boshen force-pushed the c/exhaustive-deps2 branch from a4f7675 to 171e62c Compare November 12, 2024 11:37

Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 12, 2024

Boshen force-pushed the c/exhaustive-deps2 branch from 171e62c to 3dcac1a Compare November 12, 2024 11:42

graphite-app bot merged commit 3dcac1a into main Nov 12, 2024

graphite-app bot deleted the c/exhaustive-deps2 branch November 12, 2024 11:47

oxc-bot mentioned this pull request Nov 20, 2024

release(oxlint): v0.12.0 #7366

Merged

oxc-bot mentioned this pull request Nov 21, 2024

release(crates): v0.37.0 #7399

Merged

Dunqing mentioned this pull request Jan 22, 2026

perf(linter/exhaustive-deps): simplify the logic of checking if the identifier it is a dependency of hook #18350

Merged

Uh oh!

Comments

Conversation

camc314 commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot commented Nov 6, 2024

Your org has enabled the Graphite merge queue for merging into main

Uh oh!

Uh oh!

camc314 commented Nov 7, 2024

Uh oh!

Boshen commented Nov 8, 2024

Uh oh!

camchenry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Boshen commented Nov 9, 2024

Uh oh!

camchenry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camc314 commented Nov 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Nov 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #7151 will not alter performance

Summary

Uh oh!

Uh oh!

Boshen commented Nov 11, 2024 • edited by graphite-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Boshen commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Boshen commented Nov 11, 2024

Uh oh!

Boshen commented Nov 11, 2024

Uh oh!

Boshen commented Nov 11, 2024

Uh oh!

Boshen commented Nov 11, 2024

Uh oh!

camc314 commented Nov 11, 2024

Uh oh!

Uh oh!

Uh oh!

camc314 commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camc314 commented Nov 11, 2024

Uh oh!

Boshen commented Nov 12, 2024

Uh oh!

Boshen left a comment

Choose a reason for hiding this comment

Uh oh!

camc314 commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camc314 commented Nov 12, 2024

Uh oh!

Boshen commented Nov 12, 2024

Uh oh!

Reviewers

Assignees

camc314 commented Nov 6, 2024 •

edited

Loading

camc314 commented Nov 9, 2024 •

edited

Loading

codspeed-hq bot commented Nov 9, 2024 •

edited

Loading

Boshen commented Nov 11, 2024 •

edited by graphite-app bot

Loading

Boshen commented Nov 11, 2024 •

edited

Loading

camc314 commented Nov 11, 2024 •

edited

Loading

camc314 commented Nov 12, 2024 •

edited

Loading