Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lint): support some of wrapped hook calls in useExhaustiveDependencies #4964

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

siketyan
Copy link
Contributor

Summary

Fixes #1597 (except more edge cases)

This pull request adds support of some different styles of hook calls that are considered as "stable", including:

  • Parenthesized calls (useRef())
  • Sequencial calls with a comma operator (doSomething(), useRef())
  • Calls with a TS Non-null assertion useRef()!
  • Calls with a TS as operator useRef() as T
  • Calls with a TS satisfies operator useRef() satisfies T

Note this doesn't cover all of edge cases.

Test Plan

Tests added.

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Jan 24, 2025
@siketyan siketyan changed the title fix(lint): Support some of wrapped hook calls in useExhaustiveDependencies fix(lint): support some of wrapped hook calls in useExhaustiveDependencies Jan 24, 2025
Copy link

codspeed-hq bot commented Jan 24, 2025

CodSpeed Performance Report

Merging #4964 will not alter performance

Comparing siketyan:GH-1597 (45e9668) with next (44d3f81)

Summary

✅ 95 untouched benchmarks

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Could you rebase this against the next branch and add a changeset entry? Thanks!

@siketyan
Copy link
Contributor Author

siketyan commented Jan 24, 2025

@arendjr Thank you for your review. I will rebase it soon!
Out of curiosity, should we create pull requests to next branch even if it does not introduce any new rules? If so, it will be great if that is noted in the pull request template or somewhere then we can notice it :)

@siketyan siketyan changed the base branch from main to next January 24, 2025 15:41
@ematipico
Copy link
Member

ematipico commented Jan 24, 2025

should we create pull requests to next branch

Yes, that's preferable. We did a bunch of internal refactoring, even to internal Rust methods and APIs

If so, it will be great if that is noted in the pull request template or somewhere then we can notice it :)

I wanted to update the README.md, and add a note about it

@siketyan
Copy link
Contributor Author

@ematipico Thank you for clarification!

And I rebased to next branch & added a changeset. 🙏

…ncies

Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
Comment on lines 479 to 483
fn unwrap_to_call_expression(expression: AnyJsExpression) -> Option<JsCallExpression> {
match expression {
AnyJsExpression::JsCallExpression(expr) => Some(expr),
AnyJsExpression::JsParenthesizedExpression(expr) => {
expr.expression().ok().and_then(unwrap_to_call_expression)
Copy link
Member

Choose a reason for hiding this comment

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

This is a recursion, which we tend to avoid when working with CSTs. Can you make it a loop? It shouldn't be too difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ematipico Sorry, I didn't know that. I turned it into a loop in 45e9668.

Signed-off-by: Naoki Ikeguchi <me@s6n.jp>
@siketyan siketyan requested a review from ematipico January 24, 2025 17:10
@arendjr arendjr merged commit e750523 into biomejs:next Jan 24, 2025
11 checks passed
@arendjr
Copy link
Contributor

arendjr commented Jan 24, 2025

Thanks!

@siketyan siketyan deleted the GH-1597 branch January 26, 2025 10:57
unvalley pushed a commit to unvalley/biome that referenced this pull request Jan 26, 2025
…ncies (biomejs#4964)

Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
Signed-off-by: Naoki Ikeguchi <me@s6n.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💅 Biome throw useExhaustiveDependencies error when useRef type is defined using type assertion
3 participants