-
-
Notifications
You must be signed in to change notification settings - Fork 792
feat(lint): implement useObjectSpread rule
#6129
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
Conversation
ef07839 to
4863080
Compare
CodSpeed Performance ReportMerging #6129 will not alter performanceComparing Summary
|
ematipico
left a comment
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 diagnostic doesn't follow the rule pillars. We must change it. Also, we could add more tests
| "Object spread should be used instead of "<Emphasis>"Object.assign"</Emphasis> | ||
| " when constructing new objects." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| "Replace "<Emphasis>"Object.assign({...}, <object>)"</Emphasis> | ||
| " with "<Emphasis>"{ ...<object> }"</Emphasis>"." |
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 diagnostics don't follow the three rule pillars https://biomejs.dev/linter/#rule-pillars
However, I'm not sure if what's missing is the error, or the reason why it's an error. Probably the latter
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.
Changed the diagnostic, thanks.
| ({ ...foo }); | ||
| ({ ...baz, foo: 'bar' }); | ||
| Object.assign(foo, { bar: baz }); | ||
| Object.assign(foo, bar); | ||
| Object.assign(foo, { bar, baz }); | ||
| Object.assign(foo, { ...baz }); |
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 eslint rule has way more tests, can we add them? https://github.com/eslint/eslint/blob/main/tests/lib/rules/prefer-object-spread.js
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.
Added more tests from eslint.
|
I've also added the fix action. |
ematipico
left a comment
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.
Thank you!
0b079f4 to
8ee6c0b
Compare
|
Rebased. |
Co-authored-by: ematipico <[email protected]>
Summary
Prefer object spread over
Object.assign()when constructing new objects.Object spread syntax is more concise, more readable, and performs better
than
Object.assign()when creating a new object from existing objects.It also has better TypeScript integration.
Examples
Invalid
Valid
Modifying an existing object is allowed:
Test Plan
Tests are included
Closes #4319