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
[Fix] forbid-prop-types
: warn on destructured values as well
#2676
Conversation
Fixes jsx-eslint#2662. Co-authored-by: Alex Kovar <ajkovar@gmail.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>
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.
if it's not failing any tests, seems good to me.
b16f49a
to
08cdd39
Compare
@ajkovar so i notice you modified my failing test; i rebased this PR so that it's 1 commit, and we have both tests. The one that's failing still needs fixing, though. |
Sure. Thank you for reviewing. I think I had assumed there was a typo. Let me take a look and see. |
Ok. I see now. Investigating. Thanks for adding the additional test. |
Alright. So I fixed those original test cases. I think it might be necessary to tweak the algorithm a bit though to make it recursive or something of that nature. That I think would require a larger refactor so I held off for now. I added another test case to demonstrate what I mean. There might be an easier shortcut... recursion may overkill as probably in most cases people are not going that deep with the prop types. Would be interested to hear your thoughts. |
861fdc4
to
eac5a43
Compare
Ok. I have an update for you guys. After reviewing the code further, my thoughts about recursion seem to be unnecessary since es-lint is already recursively descending through the AST. I added another selector to handle calls to the 'shape' function specifically. I think this should be sufficient but if anyone can think of why not, I would love to hear more about it. All tests in forbid-prop-types now pass (although tests seem to be broken on master). In this process, this seemed to fix another test that was passing but in a broken way (with a comment). This does not fix #1673. It doesn't seem to me that the test was accurately covering what was being requested in that ticket. That test should have failed but for a different reason ('PropTypes.object' is forbidden). |
lib/rules/forbid-prop-types.js
Outdated
if ((node.callee.name === 'shape' | ||
|| astUtil.getPropertyName(node.callee) === 'shape') | ||
&& node.arguments.length > 0) { |
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.
if ((node.callee.name === 'shape' | |
|| astUtil.getPropertyName(node.callee) === 'shape') | |
&& node.arguments.length > 0) { | |
if ( | |
node.arguments.length > 0 | |
&& (node.callee.name === 'shape' || astUtil.getPropertyName(node.callee) === 'shape') | |
) { |
319722f
to
997a99f
Compare
forbid-prop-types
: warn on destructured values as well
In this case the type of the callee was 'Identifier' (vs 'MemberExpression'), which seems to makes sense because it is not being called as a property on PropTypes object. I looked back at the history to see if maybe that check was added for a particular reason but it seems like it has been there from the beginning:
00f716a
I can't think of any reason why it would be needed but if someone who knows better than me does, I'd be happy to hear it.