Skip to content
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: allow array spread for prefer-object-spread rule (fixes #10344) #10347

Merged
merged 1 commit into from
May 17, 2018
Merged

Fix: allow array spread for prefer-object-spread rule (fixes #10344) #10347

merged 1 commit into from
May 17, 2018

Conversation

g-plane
Copy link
Member

@g-plane g-plane commented May 13, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] 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:

#10344

What changes did you make? (Give an overview)
Allow, eg:

Object.assign({}, ...objects)

Is there anything you'd like reviewers to focus on?

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 13, 2018
@aladdin-add aladdin-add added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 13, 2018
Copy link
Sponsor Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but is it possible to make Object.assign({}, foo, ...bar, {}, baz) warn and autofix to Object.assign({ ...foo }, ...bar, { ...baz })?

@platinumazure
Copy link
Member

@ljharb That almost seems like a separate change to me. Seems that we could generalize that any variables which follow object literals in an Object.assign call should be turned into object spread (if the entire Object.assign call is not transformed itself). e.g., Object.assign({}, foo) could theoretically be Object.assign({ ...foo }) if we then didn't want to optimize to { ...foo }.

Personally, I think that should be regarded as an enhancement request. This bugfix should just be about allowing Object.assign({}, ...foo).

Let me know if you strongly disagree.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contributing!

@ljharb
Copy link
Sponsor Contributor

ljharb commented May 14, 2018

(This will likely collide with #10351, but if this is merged ASAP then rebasing that one shouldn't be too tricky)

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.

@not-an-aardvark not-an-aardvark merged commit a812845 into eslint:master May 17, 2018
@g-plane g-plane deleted the fix-prefer-object-spread branch May 18, 2018 09:24
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 14, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants