Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): exhaustive deps for React hooks #3355

Merged
merged 36 commits into from
Oct 25, 2022
Merged

feat(rome_js_analyze): exhaustive deps for React hooks #3355

merged 36 commits into from
Oct 25, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Oct 6, 2022 •
edited by ematipico
Loading

Summary

This PR implements the lint rule that will check React hooks are correctly specifying dependencies.
See #3357 (comment)

Closes #3478

This PR implements

  • Missing dependencies;
  • Extra dependencies;
  • Consider some React hooks returns as stable;

Test Plan

> cargo test -p rome_js_analyze -- react_extensive_dependencies

@netlify
Copy link

netlify bot commented Oct 6, 2022 •
edited
Loading

✅ Deploy Preview for rometools ready!

Name Link
🔨 Latest commit 3b345a2
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/635728331f4b3d000a087050
😎 Deploy Preview https://deploy-preview-3355--rometools.netlify.app/docs/lint/rules
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@xunilrj xunilrj temporarily deployed to netlify-playground October 6, 2022 14:47 Inactive
@github-actions
Copy link

github-actions bot commented Oct 6, 2022 •
edited
Loading

Playground for commit 3ea3a02

@xunilrj xunilrj force-pushed the feature/extenside-deps-useEffect branch from 222dda3 to eda2ae6 Compare October 10, 2022 13:34
@xunilrj xunilrj temporarily deployed to netlify-playground October 10, 2022 13:34 Inactive
@xunilrj
Copy link
Contributor Author

xunilrj commented Oct 10, 2022

!bench_analyzer

@github-actions
Copy link

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      3.0±0.09ms     3.9 MB/sec    1.02      3.1±0.11ms     3.8 MB/sec
analyzer/index.js         1.00      8.8±0.48ms     3.7 MB/sec    1.01      8.9±0.34ms     3.7 MB/sec
analyzer/lint.ts          1.02      4.1±0.17ms    10.3 MB/sec    1.00      4.0±0.14ms    10.4 MB/sec
analyzer/parser.ts        1.00     10.1±0.43ms     4.8 MB/sec    1.04     10.6±0.45ms     4.6 MB/sec
analyzer/router.ts        1.00      7.1±0.22ms     8.6 MB/sec    1.07      7.6±0.40ms     8.0 MB/sec
analyzer/statement.ts     1.00     10.7±0.47ms     3.3 MB/sec    1.03     11.1±0.38ms     3.2 MB/sec
analyzer/typescript.ts    1.00     17.0±0.72ms     3.2 MB/sec    1.03     17.6±0.73ms     3.1 MB/sec

@xunilrj xunilrj force-pushed the feature/extenside-deps-useEffect branch 2 times, most recently from 1fe5806 to b604dd5 Compare October 12, 2022 17:26
@xunilrj xunilrj temporarily deployed to netlify-playground October 12, 2022 17:26 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 12, 2022 17:45 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 12, 2022 18:46 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 12, 2022 19:05 Inactive
@xunilrj
Copy link
Contributor Author

xunilrj commented Oct 12, 2022

!bench_analyzer

@github-actions
Copy link

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.3±0.01ms     5.1 MB/sec    1.01      2.3±0.01ms     5.0 MB/sec
analyzer/index.js         1.00      6.5±0.01ms     5.0 MB/sec    1.01      6.6±0.01ms     4.9 MB/sec
analyzer/lint.ts          1.00      3.0±0.01ms    13.8 MB/sec    1.00      3.0±0.01ms    13.8 MB/sec
analyzer/parser.ts        1.00      7.5±0.01ms     6.5 MB/sec    1.01      7.6±0.02ms     6.4 MB/sec
analyzer/router.ts        1.00      5.5±0.01ms    11.1 MB/sec    1.01      5.6±0.01ms    11.0 MB/sec
analyzer/statement.ts     1.00      8.2±0.02ms     4.3 MB/sec    1.02      8.3±0.03ms     4.3 MB/sec
analyzer/typescript.ts    1.00     12.9±0.06ms     4.2 MB/sec    1.02     13.2±0.14ms     4.1 MB/sec

@MichaReiser
Copy link
Contributor

MichaReiser commented Oct 12, 2022 •
edited
Loading

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.3±0.01ms     5.1 MB/sec    1.01      2.3±0.01ms     5.0 MB/sec
analyzer/index.js         1.00      6.5±0.01ms     5.0 MB/sec    1.01      6.6±0.01ms     4.9 MB/sec
analyzer/lint.ts          1.00      3.0±0.01ms    13.8 MB/sec    1.00      3.0±0.01ms    13.8 MB/sec
analyzer/parser.ts        1.00      7.5±0.01ms     6.5 MB/sec    1.01      7.6±0.02ms     6.4 MB/sec
analyzer/router.ts        1.00      5.5±0.01ms    11.1 MB/sec    1.01      5.6±0.01ms    11.0 MB/sec
analyzer/statement.ts     1.00      8.2±0.02ms     4.3 MB/sec    1.02      8.3±0.03ms     4.3 MB/sec
analyzer/typescript.ts    1.00     12.9±0.06ms     4.2 MB/sec    1.02     13.2±0.14ms     4.1 MB/sec

Ema mentioned on an other PR that nursery rules are probably not run as part of our benchmarks. It probably makes sense to double check that and potentially fix in a separate PR to get clear signal on perf.

@xunilrj xunilrj temporarily deployed to netlify-playground October 12, 2022 23:07 Inactive
@xunilrj xunilrj marked this pull request as ready for review October 13, 2022 11:52
@xunilrj xunilrj requested a review from a team October 13, 2022 11:52
@xunilrj xunilrj changed the title feat(rome_js_analyze): extenside deps for React feat(rome_js_analyze): extensise deps for React hooks Oct 13, 2022
@xunilrj xunilrj mentioned this pull request Oct 13, 2022
Closed
@ematipico ematipico added this to the 10.0.0 milestone Oct 13, 2022
@ematipico ematipico added the A-Linter Area: linter label Oct 13, 2022
ematipico
ematipico reviewed Oct 13, 2022
View reviewed changes
Copy link
Contributor

@ematipico ematipico left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed a bit the PR, I will do a second pass later. There's one thing that I am struggling with. We use "capture" a lot... but this is a concept I am not familiar with. Can we explain it somewhere? I checked the struct Capture and the explanation doesn't help me. Can we explain it better? I wouldn't be able to help if I don't understand the concept.

crates/rome_js_analyze/src/semantic_analyzers/nursery/react_extensive_dependencies.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/nursery/react_extensive_dependencies.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/nursery/react_extensive_dependencies.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/utils/react.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/nursery/react_extensive_dependencies.rs Outdated Show resolved Hide resolved
...rome_js_analyze/tests/specs/nursery/reactExtensiveDependencies/missingDependenciesInvalid.js Outdated Show resolved Hide resolved
crates/rome_js_semantic/src/semantic_model.rs Show resolved Hide resolved
crates/rome_js_semantic/src/semantic_model/closure.rs Outdated Show resolved Hide resolved
crates/rome_js_semantic/src/semantic_model/closure.rs Outdated Show resolved Hide resolved
crates/rome_js_semantic/src/semantic_model/closure.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser changed the title feat(rome_js_analyze): extensise deps for React hooks feat(rome_js_analyze): extensive deps for React hooks Oct 13, 2022
@xunilrj xunilrj force-pushed the feature/extenside-deps-useEffect branch from e2a3417 to 63525d4 Compare October 19, 2022 11:03
@xunilrj xunilrj force-pushed the feature/extenside-deps-useEffect branch from 394edf4 to 3b345a2 Compare October 25, 2022 00:05
@xunilrj xunilrj temporarily deployed to netlify-playground October 25, 2022 00:05 Inactive
@ematipico
Copy link
Contributor

@MichaReiser could you give a second look to this PR? It would be great to have it merged for the 10.0.0 release

@xunilrj xunilrj merged commit 65b4677 into main Oct 25, 2022
@xunilrj xunilrj deleted the feature/extenside-deps-useEffect branch October 25, 2022 12:27
@xunilrj xunilrj mentioned this pull request Oct 25, 2022
Closed
11 tasks
@ematipico ematipico mentioned this pull request Nov 15, 2022
Closed
29 tasks
arendjr
arendjr reviewed Nov 28, 2022
View reviewed changes
crates/rome_js_analyze/src/react/hooks.rs
/// Name of the React hook
hook_name: String,
/// Index of the position of the stable return, [None] if
/// none returns are stable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc issue: [None] if the entire return value is stable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@arendjr arendjr arendjr left review comments

@ematipico ematipico ematipico approved these changes

@MichaReiser MichaReiser MichaReiser approved these changes

@leops leops Awaiting requested review from leops

Assignees
No one assigned
Labels
A-Linter Area: linter
Projects
Rome 2022
Status: Done
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

Basic Implementation of rule exhaustive-deps rule
4 participants
@xunilrj @MichaReiser @ematipico @arendjr