Fix prop-types desctructuring with properties as string.
#124
Conversation
| ' render() {', | ||
| ' var { ', | ||
| ' propX,', | ||
| ' "aria-controls": ariaControls, ', |
|
It works. Very nice ! 👍 🎉 🌈 |
| return key.name; | ||
| } | ||
| return key.value; | ||
| } |
There was a problem hiding this comment.
why not
return (key.type === 'Identifier') ? key.name : key.value;here https://github.com/yannickcr/eslint-plugin-react/pull/123/files#diff-6149d4075a6f3125f18ce8c87bb253cbR39 you've done it like that:
return node.key.type === 'Identifier' ? node.key.name : node.key.value;Of course it is just a matter of taste 🍒
There was a problem hiding this comment.
The difference is that in jsx-sort-prop-types it only checks the propTypes declaration and not its uses.
I was thinking (though I didn't implement it yet) that when checking the uses, you could have the case const aria = this.props["aria-controls"];. Right now, this is ignored by the rule because it's a "computed" property. However, I could easily inspect that property, realize it's a literal string value and add the check (there are other implications which is why I haven't done it yet, mostly with case this.props["some.thing"] and name.split(".") in the rule).
Therefore, I left it with that structure to make it easy to add more checks.
Note that it's possible, but make no sense to do propType: {["aria-controls"]: React.PropTypes.string}; So they are deliberately ignored in the other rule.
There was a problem hiding this comment.
Yep, there is really a lot of different ways to declare/use propTypes, it become hard to check all of them.
There was a problem hiding this comment.
Thank you for the explanation 🍒 😃
Fix `prop-types` desctructuring with properties as string (fixes #118)
|
Awesome! Thanks! |
Fixes #118
I saw that destructuring were not correctly detected when the first key was a string and also those were not handled properly.