Add allowInPropTypes option to forbid-foreign-prop-types (#1647) #1655
Conversation
| return false; | ||
| } | ||
|
|
||
| if (node.parent.parent.parent.parent.type === 'AssignmentExpression') { |
There was a problem hiding this comment.
are we sure that the node will always have 4 parents?
There was a problem hiding this comment.
Also, rather than checking for AssignmentExpressions, is there a way we could create a visitor for AssignmentExpression instead?
There was a problem hiding this comment.
No, I'm not sure the node will always have 4 parents. I'm sure there's a better way to handle this but I don't know what it is. Is there a method I can use to search the tree for a parent that's an AssignmentExpression?
Creating a visitor for AssignmentExpression might be better but I'm not sure how to coordinate between the two visitors. If we find an AssignmentExpression where x.propTypes is on the left and allowInPropTypes is true then we need to ignore the rest of that block of code but the MemberExpression visitor will still be called for expressions inside that block of code. I'm not sure how to solve that.
There was a problem hiding this comment.
Good question.
Maybe we should make a shared utility function isInAssignmentExpression that walks upwards until it finds an AssignmentExpression?
There was a problem hiding this comment.
On the one hand I'm glad I didn't miss something obvious but on the other hand I wish there was an easy solution to this problem. 😀
I'll try adding a function that traverses the tree upwards until it reaches an AssignmentExpression. What are some other node types that should make it stop? I'm thinking Program and FunctionExpression at the very least. Anything else?
There was a problem hiding this comment.
Program, certainly. Not so sure about FunctionExpression.
| } | ||
|
|
||
| if (node.parent.parent.parent.parent.type === 'AssignmentExpression') { | ||
| const assignmentExpression = node.parent.parent.parent.parent; |
There was a problem hiding this comment.
might as well create this var above the conditional, so it can be reused in there?
| context.report({ | ||
| node: node.property, | ||
| message: 'Using another component\'s propTypes is forbidden' | ||
| message: | ||
| 'Using propTypes from another component is not safe because they may be removed in production builds' |
There was a problem hiding this comment.
this is a super weird line break; can this all go on one line? (line length limits aren't important)
There was a problem hiding this comment.
This would have been done by Prettier. I'll revert it.
| }); | ||
| } | ||
| }, | ||
|
|
||
| ObjectPattern: function(node) { | ||
| const propTypesNode = node.properties.find(property => property.type === 'Property' && property.key.name === 'propTypes'); | ||
| const propTypesNode = node.properties.find( | ||
| property => property.type === 'Property' && property.key.name === 'propTypes' |
There was a problem hiding this comment.
similarly, please revert this back to one line
|
|
||
| if (propTypesNode) { | ||
| context.report({ | ||
| node: propTypesNode, | ||
| message: 'Using another component\'s propTypes is forbidden' | ||
| message: | ||
| 'Using propTypes from another component is not safe because they may be removed in production builds' |
There was a problem hiding this comment.
also here, this should be one line.
|
I've improved the traversal up the tree in search of a parent |
| } else if (parent.parent) { | ||
| parent = parent.parent; | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
i think we can collapse the else if and the else together, and just do parent = parent.parent, and let the while condition catch it? (also, maybe null would make more sense than false)
ljharb
force-pushed
the
fix-1647
branch
2 times, most recently
from
98b047d
to
7443a37
Compare
|
Any idea when this will be released? I want to get it into create-react-app. |
|
v7.7.0 is released. |
Add an option to allow using another components propTypes inside a propTypes declaration.
Closes #1647