-
-
Notifications
You must be signed in to change notification settings - Fork 500
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: noInvalidUseBeforeDeclaration
false-positive for param destructuring with default values
#1672
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
CodSpeed Performance ReportMerging #1672 will degrade performances by 7.08%Falling back to comparing Summary
Benchmarks breakdown
|
@@ -136,6 +136,20 @@ impl Rule for NoInvalidUseBeforeDeclaration { | |||
// type Y = typeof X; | |||
// const X = 0; | |||
// ``` | |||
&& reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with this fix is: it doesn't report used before declaraion such as { a = b, b = "111" }
.
I wonder if we should rather change the implementation of biome_js_syntax::binding_ext::AnyJsIdentifierBinding::declaration
and consider JsObjectBindingPatternShorthandProperty
, JsObjectBindingPattern*
and JsArrayBindingPattern*
as declarations. I am not sure of the implication in other part of the codebase. This could solve both #1652 and #1648.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will look into this after some days, quite busy now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you still interested in investigating this? Otherwise, I can take a look :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Conaclos, I get the idea that we have to include JsObjectBindingPatternShorthandProperty
as declarations but as you mention I found it hard to handle its implication in other parts of codebase, so you take a look on this. I will follow your work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed a fix in #1800
Summary
Closes #1648
Test Plan
New tests are passed