-
-
Notifications
You must be signed in to change notification settings - Fork 485
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(js_formatter): format hook with 3 arguments #4462
fix(js_formatter): format hook with 3 arguments #4462
Conversation
CodSpeed Performance ReportMerging #4462 will not alter performanceComparing Summary
|
@vohoanglong0107 FYI remember to add @biomejs/core-contributors too when requesting for code review. If you check the maintainers team, it doesn't contain the members that belong to the core contributors team |
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.
You created the test in the wrong folder, the prettier folder should stay as is. Create a new test inside specs, maybe here: https://github.com/biomejs/biome/tree/main/crates%2Fbiome_js_formatter%2Ftests%2Fspecs%2Fjs%2Fmodule
@@ -31,3 +31,7 @@ expect( | |||
bar; | |||
}, | |||
).toThrow(ReferenceError); | |||
|
|||
useImperativeHandle(ref, () => { |
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.
These tests are coming from prettier repository, so they should not be modified
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.
My bad 😅 I thought that since we are dealing with a difference in formatting with prettier, I should place the test there
Summary
Currently the hook detection in JS formatter only recognizes hooks with 2 arguments. This PR updates the formatter to also recognize hooks with 3 arguments.
Fix #4262
Test Plan
Added new test with the
useImperativeHandle
hook, a 3 arguments hook.