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

react/jsx-key is triggering false positive for props defined as an object properties #1753

Closed
awthwathje opened this issue Apr 1, 2018 · 16 comments

Comments

@awthwathje
Copy link
Contributor

Sometimes it is useful to define a component call as

<Component {...{ key: id, id, shortTitle }} />

instead of

<Component key={id} id={id} shortTitle={shortTitle} />

but in the first case ESLint gets mad about the missing key prop, triggering the react/jsx-key rule.

This is the reason why I'm turning this rule off for now. I believe this should be fixed.

@ljharb
Copy link
Member

ljharb commented Apr 1, 2018

This is actually intentional, and in a future version of React key I believe won't be possible to spread, it will only be allowed statically provided.

The key should always be hardcoded; it's not a normal prop.

@ljharb ljharb closed this as completed Apr 1, 2018
@awthwathje
Copy link
Contributor Author

Then I think it is good idea to explicitly mention this in the documentation.

@ljharb
Copy link
Member

ljharb commented Apr 2, 2018

Sure, a PR to add that to the jsx-key docs would be great!

@awthwathje
Copy link
Contributor Author

Duh... Well, okay. Will do it.

awthwathje added a commit to awthwathje/eslint-plugin-react that referenced this issue Apr 3, 2018
Spreaded key is discouraged and currently triggering the rule. Added some clarification to the doc.

Resolves jsx-eslint#1753.
This was referenced Sep 22, 2018
@jeremywiebe
Copy link

@ljharb Sorry to revive an old Issue. I was wondering if you could point to some React docs that talk about this:

in a future version of React key I believe won't be possible to spread, it will only be allowed statically provided.

@ljharb
Copy link
Member

ljharb commented Sep 24, 2019

reactjs/rfcs#107

@machineghost
Copy link

machineghost commented Jun 6, 2020

Can we take a step back here? We have a request for a change to React, which may never happen ... and because of that ESLint refuses to even provide the option to mark 100% valid code (today) as valid?

How does this make sense?

@ljharb
Copy link
Member

ljharb commented Jun 6, 2020

@machineghost this is an eslint rule, as such, it's opinionated by design. The entire purpose of eslint rules is to choose which 100% valid code is marked as invalid - otherwise you'd just rely on syntax errors and not use eslint at all.

The intention of this rule is for key to be statically hardcoded inline, not spread in. If you don't like that, disable the rule.

@machineghost
Copy link

machineghost commented Jun 6, 2020

@ljharb Yes, I 100% agree: ES Lint is opinionated, by design ... but it has historically never been so opinionated as to refuse to even consider an option for (again) perfectly valid code.

On the contrary, ESLint is FULL of options to rules. There are quite literally hundreds of ways to customize various ES Lint rules in existence.

Thus, saying "The only way to get ES Lint to accept perfectly valid code is to completely disable a rule (and lose the benefit of all of the other meaningful/valid cases that it catches) ... and won't even consider adding an option" seems significantly inconsistent with the rest of the ES Lint rules to me.

@ljharb
Copy link
Member

ljharb commented Jun 6, 2020

You're certainly right that there's lots of options; but eslint core has in fact often refused to add options for otherwise valid code, as have various plugin maintainers. You're more than welcome to create your own plugin, compose any existing rule, and modify (or add to) the reports it generates (see eslint-rule-composer).

I can certainly see the rule being modified, without an option, to allow an inline spread static key (the OP's case), and I'd be happy to review a PR that did that.

@machineghost
Copy link

machineghost commented Jun 6, 2020

From "ES Lint eslint-plugin-react doesn't want your garbage (but 100% valid) code" to "happy to review a PR" in only one comment ;) Thank you!!

@machineghost
Copy link

machineghost commented Jun 6, 2020

I can certainly see the rule being modified, without an option, to allow an inline spread static key (the OP's case), and I'd be happy to review a PR that did that.

P.S. Why "without an option"? Couldn't there be users who want the current behavior? An option seems safer ...

@ljharb
Copy link
Member

ljharb commented Jun 6, 2020

That's a fair point; the option would be needed when React makes spreading key invalid, might as well add it now.

@machineghost
Copy link

machineghost commented Jun 6, 2020

That's a fair point; the option would be needed when React makes spreading key invalid, might as well add it now.

IF React makes spreading key invalid. Again, nothing has happened on that for two years, so ... maybe you shouldn't hold your breath waiting for it ;)

Never worked on an ES Lint rule before but I'll see if I can pick it up and submit a PR.

@ljharb
Copy link
Member

ljharb commented Jun 6, 2020

React 17 hasn't come out after more than 2 years of work - so far they ship almost all their plans eventually, even if on a scale of years.

@machineghost
Copy link

machineghost commented Jun 6, 2020

EDIT: Sorry, please disregard my previous post here; I think Prettier (which my VS Code is setup to run automatically) got run against the file and broke a bunch of things. Once I fixed the file it had 142 warnings ... but 0 errors :)

machineghost added a commit to machineghost/eslint-plugin-react that referenced this issue Jun 8, 2020
…d (ie. ...) arguments with keys to be considered as valid as explicit key attributes

Part of jsx-eslint#1753
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants