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

feat(rome_js_analyze): use exhaustive deps support properties #3581

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Nov 7, 2022

Summary

This PR allows useExhaustiveDependencies to support properties. This opens three new possibilities:

  • we correctly specify the property as dependency;
  • we specify a more generic dependency;
  • we specify a more specific dependency.

Test Plan

> cargo test -p rome_js_analyze -- exhaustive

@netlify
Copy link

netlify bot commented Nov 7, 2022

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit ce04744
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/637e8c1335a782000973d041

@xunilrj xunilrj temporarily deployed to netlify-playground November 7, 2022 22:03 Inactive
@xunilrj xunilrj changed the title Feature/use exhaustive deps support properties feat(rome_js_analyze): use exhaustive deps support properties Nov 7, 2022
@github-actions
Copy link

github-actions bot commented Nov 7, 2022

@MichaReiser MichaReiser added this to the 10.1.0 milestone Nov 8, 2022
@xunilrj xunilrj marked this pull request as ready for review November 8, 2022 10:24
@xunilrj xunilrj requested a review from leops as a code owner November 8, 2022 10:24
@xunilrj xunilrj mentioned this pull request Nov 8, 2022
11 tasks
@ematipico ematipico mentioned this pull request Nov 15, 2022
29 tasks
Fix::DependencyTooDeep(use_effect_range, capture, dependency) => {
let diag = RuleDiagnostic::new(
rule_category!(),
use_effect_range,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, and I don't expect to be implemented in this PR. I was wondering if we should actually use the range that starts from the hook name and finishes at ), because the issue is the whole hook, not just the name.

// from start range of `useEffect`
useEffect(() => {
	return a + 1;
}, [a]);
// to the range of the last token `)`

This is a suggestion for all the diagnostics obviously. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally tend to dislike ranges too big. Because don't focus your attention, and is horrible in the IDE. But we can do, if everyone else prefers.

@MichaReiser MichaReiser added the A-Linter Area: linter label Nov 16, 2022
@calibre-analytics
Copy link

Comparing feat(rome_js_analyze): use exhaustive deps support properties Snapshot #1 to median since last deploy of rome.tools.

LCP? CLS? TBT?
Overall
Median across all pages and test profiles
2.2s
from 258ms
0.0
no change
173ms
no change
Chrome Desktop
Chrome Desktop • Cable
2.2s
from 258ms
0.0
no change
358ms
from 22ms
iPhone, 4G LTE
iPhone 12 • 4G LTE
1.1s
from 239ms
0.0
no change
11ms
no change
Motorola Moto G Power, 3G connection
Motorola Moto G Power • Regular 3G
17s
from 1.06s
0.0
no change
173ms
no change

1 page tested

 Home

Browser previews

Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection
Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection

Most significant changes

Value Budget
JS Parse & Compile
Motorola Moto G Power, 3G connection
1.78s
from 27ms
Total JavaScript Size in Bytes
Chrome Desktop
5.35 MB
from 86.8 KB
Total JavaScript Size in Bytes
iPhone, 4G LTE
5.35 MB
from 86.8 KB
Total JavaScript Size in Bytes
Motorola Moto G Power, 3G connection
5.35 MB
from 86.8 KB
JS Parse & Compile
iPhone, 4G LTE
502ms
from 10ms

27 other significant changes: JS Parse & Compile on Chrome Desktop, Total Blocking Time on Chrome Desktop, Largest Contentful Paint on Motorola Moto G Power, 3G connection, First Contentful Paint on Motorola Moto G Power, 3G connection, Time to Interactive on Motorola Moto G Power, 3G connection, Total CSS Size in Bytes on Chrome Desktop, Total CSS Size in Bytes on iPhone, 4G LTE, Total CSS Size in Bytes on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Chrome Desktop, Number of Requests on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Largest Contentful Paint on Chrome Desktop, First Contentful Paint on Chrome Desktop, Speed Index on Motorola Moto G Power, 3G connection, Time to Interactive on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, First Contentful Paint on iPhone, 4G LTE, Speed Index on Chrome Desktop, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop

Calibre: Site dashboard | View this PR | Edit settings | View documentation

@xunilrj xunilrj merged commit 9e758af into main Nov 23, 2022
@xunilrj xunilrj deleted the feature/use-exhaustive-deps-support-properties branch November 23, 2022 21:15
jeysal added a commit to jeysal/rometools that referenced this pull request Nov 24, 2022
* upstream/main: (73 commits)
  fix(semantic_analyzers): style/noShoutyConstants does not recognize multiple uses of a constant. (rome#3789)
  feat(rome_js_analyze): useDefaultSwitchClauseLast (rome#3791)
  chore: run rustfmt and typo fix (rome#3840)
  feat(rome_js_analyze): use exhaustive deps support properties (rome#3581)
  website(docs): Fix text formatting (rome#3828)
  feat(rome_js_analyze): `noVoidTypeReturn` (rome#3806)
  feat(rome_cli): expose the `--verbose` flag to the CLI (rome#3812)
  fix(rome_diagnostics): allow diagnostic locations to be created without a resource (rome#3834)
  feat(rome_js_analyze): add noExtraNonNullAssertion rule (rome#3797)
  fix(rome_lsp): lsp friendly catch unwind (rome#3740)
  feat(rome_js_semantic): model improvements (rome#3825)
  feat(rome_json_parser): JSON Lexer (rome#3809)
  feat(rome_js_analyze): implement `noDistractingElements` (rome#3820)
  fix(rome_js_formatter): shothanded named import line break with default import (rome#3826)
  feat(rome_js_analyze): `noConstructorReturn` (rome#3805)
  feat(website): change enabledNurseryRules to All/Recommended select (rome#3810)
  feat(rome_js_analyze): noSetterReturn
  feat(rome_js_analyze): noConstructorReturn
  feat(rome_analyze): suppress rule via code actions (rome#3572)
  feat(rome_js_analyze): `noVar` (rome#3765)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants