Added support (without options) for spread arguments with keys #2664
Conversation
…d (ie. ...) arguments with keys to be considered as valid as explicit key attributes Part of jsx-eslint#1753
tests/lib/rules/jsx-key.js
Outdated
| @@ -39,6 +39,8 @@ ruleTester.run('jsx-key', rule, { | |||
| {code: 'fn()'}, | |||
| {code: '[1, 2, 3].map(function () {})'}, | |||
| {code: '<App />;'}, | |||
| {code: '[1, 2, 3].map(x => <App {...{ key }} />);'}, | |||
| {code: '[1, 2, 3].map(x => <App {...objectWithKey} />);'}, | |||
There was a problem hiding this comment.
this should fail, i think, unless we want to try to track down objectWithKey and ensure it is an object literal with a hardcoded key property.
There was a problem hiding this comment.
I'll have to confess my ignorance of ESLint here: I didn't realize that latter option was even possible.
I thought we'd either have to blanket allow or deny such spread objects .... and I'd argue there is value in letting a dev say "I use ESLint as a tool to help me catch problems, but I have no problem remembering to put keys in my objects, so I'd prefer ES Lint not bother me with pointless error messages every time I spread an object" :)
But of course, if it's possible to make ESLint do something that would make any dev happy, and only warn about the legitimately problematic cases, that'd be best. Can you point me to any rules that do something similar so I could get an idea how?
There was a problem hiding this comment.
When the object is defined in the same file, as a literal, eslint provides the tools to read it as if it was defined inline.
Also, it's often better to wrongly block something than to wrongly allow it.
I'm not sure off the top of my head which rules do similar things; i'd look into our propType detection code.
tests/lib/rules/jsx-key.js
Outdated
| @@ -39,6 +39,8 @@ ruleTester.run('jsx-key', rule, { | |||
| {code: 'fn()'}, | |||
| {code: '[1, 2, 3].map(function () {})'}, | |||
| {code: '<App />;'}, | |||
| {code: '[1, 2, 3].map(x => <App {...{ key }} />);'}, | |||
There was a problem hiding this comment.
| {code: '[1, 2, 3].map(x => <App {...{ key }} />);'}, | |
| {code: '[1, 2, 3].map(x => <App {...{ key }} />);'}, | |
| {code: '[1, 2, 3].map(x => <App {...{ key: x.id }} />);'}, | |
| {code: '[1, 2, 3].map(x => <App {...{ "key": x.id }} />);'}, | |
| {code: '[1, 2, 3].map(x => <App {...{ 'key': x.id }} />);'}, | |
| {code: '[1, 2, 3].map(x => <App {...{ ['key']: x.id }} />);'}, | |
| {code: '[1, 2, 3].map(x => <App {...{ ["key"]: x.id }} />);'}, | |
| {code: '[1, 2, 3].map(x => <App {...{ [`key`]: x.id }} />);'}, |
|
Since you never responded about the spread stuff I just made it an option, but that can easily change. |
|
From the lack of response (one way or the other) here, is it safe to assume that I will need to fork the library if I want this change published (and just in general if I want to see other improvements made to it)? |
|
@machineghost sorry for the delay; what are you still waiting for a response on? I do have a reply here that you haven't replied to yet? |
ljharb
force-pushed
the
master
branch
6 times, most recently
from
59af733
to
865ed16
Compare
ljharb
force-pushed
the
master
branch
4 times, most recently
from
069314a
to
181c68f
Compare
This is just a first step to try and solve: #1753
This is ONLY a request for code review: please do not merge. I just wanted to get feedback on what I have so far before I add in options. Also I wanted to get your thoughts on whether we should have two options or just one.
It seems like we definitely want to guard this new flexibility with some sort of "allow spread keys" option, but then do we want just one, or do we want to allow explicit spread keys:
separately from implicit ones:
Hypothetically speaking I could see a dev wanting to allow the former but not the latter ... but maybe that's YAGNI?