-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure that the node will always have 4 parents?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also here, this should be one line.
I've improved the traversal up the tree in search of a parent |
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.
LGTM, just one more comment
} else if (parent.parent) { | ||
parent = parent.parent; | ||
} else { | ||
return false; |
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.
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
)
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