-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Changes rationale for Object.assign #10621
Conversation
Hi @bennypowers, thanks for the PR. The new wording feels a little opinionated to me. Wondering if it might be better just to suggest that the rule could be disabled when object spread is not available for the user's codebase? |
@platinumazure I'm ok with that in principle: my main issue was the phrase "syntactic sugar" which implied that both forms are entirely equivalent. OTOH I've often learned nice little tidbits from eslint docs, which is why I wanted to offer users a more concrete example of the advantages of the assign method |
@bennypowers Fair enough. I'm wondering if maybe the introduction of the rule could be a good place to go into the benefits of object spread vs |
@platinumazure it's my first rodeo in eslint docs land, so I'll defer to your judgement. Will revise for your review a little later on. |
Ok I updated the wording below and added some context above. Please review and let me know if this is more in line with what you had in mind. |
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.
Just one small change, otherwise LGTM, thanks!
docs/rules/prefer-object-spread.md
Outdated
@@ -2,6 +2,8 @@ | |||
|
|||
When Object.assign is called using an object literal as the first argument, this rule requires using the object spread syntax instead. This rule also warns on cases where an `Object.assign` call is made using a single argument that is an object literal, in this case, the `Object.assign` call is not needed. | |||
|
|||
Object spread is a declarative alternative which may perform better than the more dynamic, imperative `Object.assign` |
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.
Could you please add a period to the end of this sentence?
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.
good catch. pushed.
Object spread is not just syntax sugar. It actually performs better in browsers. The "When Not To Use It" section should therefore give a more nuanced opinion on when Object.assign is desireable.
The wording is fine, but performance is entirely unrelated to whether it’s syntax sugar. |
@bennypowers I apologize for the delay in reviewing again. My work week was terrible and I didn't have much time for open-source activities. @ljharb Could you please clarify your comment? |
Something being syntax sugar is unrelated to its performance. I would rephrase this as “object spread should be preferred not just because it’s clearer syntax sugar but also because it performs better in browsers” |
I'm cool with @ljharb phrasing. Can we remove the word "sugar"? In a speculative 5-years-from-now, I could imagine spread considered the idiom, and Object.assign a specialty function. |
It's still forever going to be syntax sugar - since it's syntax that replicates the functionality of API. It's not called sugar because of being idiomatic, it's an actual definition. |
@ljharb You're quoting from the initial issue post. Do you have any issue with the actual commit? |
@platinumazure nope, but I think it's important to clarify the well-understood definition of "syntax sugar" in the PR, for future readers :-) the commit is fine. |
Thanks for clarifying @ljharb. With that, I'm going to go ahead and merge this. Thanks @bennypowers for contributing! |
object spread is not just syntax sugar but actually performs better in browsers. The "When Not To Use It" section should therefore give a more nuanced opinion on when Object.assign is desireable.
What is the purpose of this pull request? (put an "X" next to item)
[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Changed the "When Not To Use It" section of the
prefer-object-spread
docs.Is there anything you'd like reviewers to focus on?
More concrete ideas for positive uses for
Object.assign