-
-
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
Rule request: prefer-object-spread
#7230
Comments
Object spread is still stage 2, right? It's one thing for us to have the parser option, but I'm not sure if we want to have rules around it just yet. |
@platinumazure yes, it's still stage 2. My hope is that it will achieve stage 3 next week - at that point, would it be OK to have the rule? |
Well, our policy is usually to wait until stage 4 (cf. async/await), but given that we have a parser option already... I have no idea what might happen in this case. 😄 |
In my opinion, this might be better as an |
@not-an-aardvark that would be totally fine too! As long as there's a way to enforce it. |
I think a new option for |
As of this week, object rest/spread is stage 3, so it's just a matter of time :-) |
At first glance, this seems more appropriate as a custom rule. prefer-spread is a very narrow use case right now, and these changes don't fit well there. I'm also not sure about the relationship between Object.assign and spread properties as something that is generic enough to be included in core. So, Id suggest creating a custom rule for this. |
@nzakas spread props are, by spec, a 1:1 mapping to "Object.assign with an empty object literal as the target argument" |
@ljharb I get that. My point is that I'm not sure there's much of a value proposition for using spread properties instead of Since we have a very high bar for including rules in the core, we really look for rules that have an obvious and easily discernible value proposition to end users. I just don't feel like this proposal meets that high bar. |
The value is that it's syntax, instead of API, so it's a) harder to mess up (ie, by forgetting to pass an empty object as the first argument, thus accidentally mutating the first argument), and b) easier for engines to optimize. |
Avoiding accidental mutation imo is a pretty high value prop, much higher than enforcing indentation or trailing commas (which are also important, ofc), to name a few :-/ |
@ljharb I understand that you feel it's important and useful, and the good news is that you can make this rule for yourself. I still don't see it as important enough to be in core at this point. |
I don't understand that - why is |
I agree with @ljharb that this rule would be a good idea, but I think it would be better as an additional option for |
@ljharb I've already explained my position in a previous comment. And as our guidelines say, we are being very picky about new rules because we already have way too many in the core and people find it daunting. If other team members feel strongly about it, I'll withdraw my objection. Just, to me, this is one of those preferences that offers no clear benefit to users. It's a stylistic preference for a feature that isn't yet included in the spec, and as such, I have a hard time rationalizing its inclusion. |
I suppose there are two independent questions here:
|
The TSC decided in the 2016-10-13 meeting not to accept this option into core. |
Speaking for myself, I would use this if someone were to implement it in a plugin! |
whoa, can we get some more context on this decision? |
Thanks - I find that very sad. There is a clear level of indirection and footgun this avoids, which is "forgetting to use |
Just for anyone finding this discussion through search, there is a plugin available for this here. |
Object rest/spread is now stage 4 (#9885), perhaps this could be reopened? |
I'll champion this proposal, new |
@mysticatea we already have an implementation of this rule internally at @airbnb, mind if we submit a PR? |
@ljharb go ahead, but keep in mind the issue is not accepted yet. There was an ongoing conversation on the scope of the rule that I think still has to be resolved |
Added a 👍 so this should theoretically be accepted now. @alberto What discussion are we having about scoping? I seem to have missed that. |
@platinumazure i was talking about @not-an-aardvark comment #7230 (comment) |
... |
@stevenvachon that’s a wildly unhelpful comment; there’s an open PR and it’s being worked on. |
* New: Adds prefer-object-spread rule (refs: #7230) * Adds exception for `Object.assign` with one object literal argument This was an exception to this rule that we use internally, in the case that an `Object.assign` call is made with an object literal as the only argument. The `Object.assign` call is not needed and we can just use the object literal directly. * Refactors and adds multiline object fixes * Many fixes and improvements: - handles nested object literals - handles various comment types - places comma after arguments in a smarter way * Fixes typo and improves wording in docs * include string.prototype.matchall in package.json * Several improvements: - adds line and column numbers to tests - warn on cases where argument is a spread element, - use getCommentsInside instead of regex - fix example in doc * use spread instead of apply * Check if the native `Object` is being overwritten, if it is, do not warn. Bug fix to ensure that an we're warning on an `Object.assign` for the literal case. * make messages consistent
Closing as #9955 was merged. |
Please describe what the rule should do:
When it finds an
Object.assign
call where the first argument is an object literal, the rule should error, and recommend using the object spread operator instead.What category of rule is this? (place an "X" next to just one item)
[ ] Enforces code style
[ ] Warns about a potential error
[x] Suggests an alternate way of doing something
[ ] Other (please specify:)
Provide 2-3 code examples that this rule will warn about:
Why should this rule be included in ESLint (instead of a plugin)?
Just like http://eslint.org/docs/rules/prefer-spread, this recommends that users use a syntax feature (an operator) instead of API (Object.assign) for the cases where it makes sense. Both are core language features, and eslint core should be where core language feature rules live.
The text was updated successfully, but these errors were encountered: