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.

Sign up for GitHub

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

Jump to bottom

Added support (without options) for spread arguments with keys #2664

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Added support (without options) for spread arguments with keys #2664

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27cb8bf
Added support (without options ... which are still to come) for sprea…
machineghost Jun 8, 2020
8be1cd3
Added allowSpreadKeys option to control whether spread-based keys are…
machineghost Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 19 additions & 1 deletion lib/rules/jsx-key.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,25 @@ module.exports = {
const fragmentPragma = pragmaUtil.getFragmentFromContext(context);

function checkIteratorElement(node) {
if (node.type === 'JSXElement' && !hasProp(node.openingElement.attributes, 'key')) {
if (node.type === 'JSXElement') {
const hasAKeyAttribute = hasProp(node.openingElement.attributes, 'key');

const hasASpreadArgumentWithAKeyProperty = node.openingElement
&& node.openingElement.attributes
&& node.openingElement.attributes.some(
({argument}) => argument && argument.properties && argument.properties.some(
(property) => property.key.name === 'key'));

const hasAnObjectSpreadArgument = node.openingElement
&& node.openingElement.attributes
&& node.openingElement.attributes.some(
({argument}) => argument && argument.type === 'Identifier');

const isValidElement = hasAKeyAttribute
|| hasASpreadArgumentWithAKeyProperty
|| hasAnObjectSpreadArgument;
if (isValidElement) return;

context.report({
node,
message: 'Missing "key" prop for element in iterator'
Expand Down
2 changes: 2 additions & 0 deletions tests/lib/rules/jsx-key.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }} />);'},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{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 }} />);'},

{code: '[1, 2, 3].map(x => <App {...objectWithKey} />);'},
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

{code: '[<App key={0} />, <App key={1} />];'},
{code: '[1, 2, 3].map(function(x) { return <App key={x} /> });'},
{code: '[1, 2, 3].map(x => <App key={x} />);'},
Expand Down