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

Add option to no-unused-vars when using object spreads to omit properties #6563

Closed
basicdays opened this issue Jun 29, 2016 · 16 comments
Closed
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@basicdays
Copy link

basicdays commented Jun 29, 2016

What version of ESLint are you using?
2.13.1

What rule do you want to change?
I'd like to propose a new option to the no-unused-vars rule that does not flag variables used in situations when using the Object Spread Properties when omitting properties while copying objects.

The ES proposal for the Stage 2 Object Rest/Spread Properties enhancement can be found here: https://github.com/sebmarkbage/ecmascript-rest-spread

What code should be flagged as correct with this change?
The following code has an object data that will eventually reach a point where I'd like to copy the object and omit specific properties. In this case I'm copying the properties into copiedData and omitting property one.

let data = {one: 1, two: 2, three: 3};
//...

let {one, ...copiedData} = data;

What happens when the rule is applied to this code now?
It will specify one as an unused variable. While this is true, having an option to disregard unused variables when used with an omit pattern would be great to have. One area that I've used this pattern quite often is in React when passing properties from one component to the next and omitting properties along the way.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jun 29, 2016
@platinumazure
Copy link
Member

Hi @basicdays, it looks like you're proposing a rule change. Could you please augment your original post to answer the questions asked on the linked documentation? Thanks!

@platinumazure platinumazure added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion needs info Not enough information has been provided to triage this issue and removed triage An ESLint team member will look at this issue soon labels Jun 29, 2016
@eslintbot
Copy link

Hi @basicdays, thanks for the issue. It looks like there's not enough information for us to know how to help you. If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

@basicdays
Copy link
Author

@platinumazure Thanks for pointing that out, I've updated the initial post.

@ilyavolodin ilyavolodin removed the needs info Not enough information has been provided to triage this issue label Jun 30, 2016
@basicdays
Copy link
Author

Just wanted to also give a heads up that apparently this pattern is getting recommended in the React documentation as well: https://facebook.github.io/react/warnings/unknown-prop.html

@platinumazure
Copy link
Member

platinumazure commented Jul 8, 2016

Awesome, thanks for editing your post. Seems reasonable to me as long as it's been behind an option. Let's see what the team thinks.

To clarify: I'm 👍 for this contingent on it being behind an option.

@alberto
Copy link
Member

alberto commented Aug 1, 2016

Why don't you write it as:

let {one} = data;

instead?

@basicdays
Copy link
Author

@alberto good question. The end goal of this is to actually get all the rest of the props from data, minus one. In this case, what I'd actually need is the inverse of what you have as an example.

@alberto
Copy link
Member

alberto commented Aug 1, 2016

Oh, sorry I didn't read carefully enough and misunderstood the issue. It seems reasonable to me.

@mysticatea
Copy link
Member

@alberto To use copiedData. He will use rest property syntax to copy with deleting a property.

Though I think too early to add an option for the stage 2 proposal.

@alberto
Copy link
Member

alberto commented Aug 1, 2016

Mmm, good point.

@basicdays
Copy link
Author

I can certainly understand shelving the idea until the proposal rises to higher stages. For now I've just been doing something like the following example to tell the linter to not flag it:

jquense/react-formal@0551ca0

I'd imagine a lot of people are probably going to be using this pattern over time, assuming the proposal doesn't get derailed. :)

Is there a way to make it a plugin? I wouldn't mind finally looking at how eslint works if you all want to hold off integrating it into eslint proper.

@alberto
Copy link
Member

alberto commented Aug 2, 2016

Yes, you can copy and alter the rule as a plugin.

Closing for now.

@alberto alberto closed this as completed Aug 2, 2016
@mysticatea
Copy link
Member

mysticatea commented Aug 2, 2016

I guess you can use context.markVariableAsUsed method for this.

http://eslint.org/docs/developer-guide/working-with-rules#the-context-object

This is similar to react/jsx-uses-vars rule.

@basicdays
Copy link
Author

Sounds good, I'll look into it. Thanks for the feedback.

@mnpenner
Copy link

mnpenner commented Sep 9, 2016

Do we have a 3rd-party solution to this yet? I'd like this feature too. I use object spread to omit vars all the time; it's very common in React.

@basicdays
Copy link
Author

Wish I've had time to write something for this, but haven't been able to get into it yet. I haven't looked to see if anyone else have written something yet though.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 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 Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

7 participants