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
[New]: jsx-key
: added checkKeyMustBeforeSpread
option for new jsx transform
#2835
Conversation
@ljharb should be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The default can't be true, or that'd be a breaking change.
lib/rules/jsx-key.js
Outdated
let keyAttributeIndex = -1; | ||
let spreadAttributeIndex = -1; | ||
|
||
attributes.forEach((attribute, i) => { | ||
if (attribute.type === 'JSXSpreadAttribute') { | ||
spreadAttributeIndex = i; | ||
} | ||
|
||
if (attribute.type === 'JSXAttribute') { | ||
if (propName(attribute) === 'key') { | ||
keyAttributeIndex = i; | ||
} | ||
} | ||
}); | ||
|
||
return spreadAttributeIndex !== -1 && keyAttributeIndex > spreadAttributeIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let keyAttributeIndex = -1; | |
let spreadAttributeIndex = -1; | |
attributes.forEach((attribute, i) => { | |
if (attribute.type === 'JSXSpreadAttribute') { | |
spreadAttributeIndex = i; | |
} | |
if (attribute.type === 'JSXAttribute') { | |
if (propName(attribute) === 'key') { | |
keyAttributeIndex = i; | |
} | |
} | |
}); | |
return spreadAttributeIndex !== -1 && keyAttributeIndex > spreadAttributeIndex; | |
var hasFoundSpread = false; | |
return attributes.some((attribute) => { | |
if (attribute.type === 'JSXSpreadAttribute') { | |
hasFoundSpread = true; | |
return false; | |
} | |
if (attribute.type !== 'JSXAttribute') { | |
return false; | |
} | |
return !hasFoundSpread || propName(attribute) === 'key'; | |
}); |
what about this alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasFoundSpread && propName(attribute) === 'key'
may be clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, in that case you'd need to negate the overarching .some
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great! If there's further tweaking needed for the TS tests in older eslint/node, I'll handle that.
jsx-key
: added checkKeyMustBeforeSpread
option for new jsx transform
fix #2830
related: facebook/react#20031 (comment)