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.

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

Merged
merged 1 commit into from Jun 27, 2020

Conversation

ajkovar
Copy link
Contributor

@ajkovar ajkovar commented Jun 16, 2020

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.

Fixes jsx-eslint#2662.

Co-authored-by: Alex Kovar <ajkovar@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Copy link
Member

@ljharb ljharb left a 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.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2020

@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.

@ajkovar
Copy link
Contributor Author

ajkovar commented Jun 16, 2020

Sure. Thank you for reviewing. I think I had assumed there was a typo. Let me take a look and see.

@ajkovar
Copy link
Contributor Author

ajkovar commented Jun 16, 2020

Ok. I see now. Investigating. Thanks for adding the additional test.

@ajkovar
Copy link
Contributor Author

ajkovar commented Jun 16, 2020

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.

@ajkovar
Copy link
Contributor Author

ajkovar commented Jun 23, 2020

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).

Comment on lines 169 to 171
if ((node.callee.name === 'shape'
|| astUtil.getPropertyName(node.callee) === 'shape')
&& node.arguments.length > 0) {
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
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')
) {

@ljharb ljharb force-pushed the forbid-objectof-2662 branch 2 times, most recently from 319722f to 997a99f Compare June 27, 2020 08:09
@ljharb ljharb merged commit 997a99f into jsx-eslint:master Jun 27, 2020
@ljharb ljharb changed the title Forbid objectof 2662 [Fix] forbid-prop-types: warn on destructured values as well Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants