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
Extract propTypes detection code #1911
Extract propTypes detection code #1911
Conversation
lib/rules/no-unused-prop-types.js
Outdated
@@ -893,7 +454,7 @@ module.exports = { | |||
return; | |||
} | |||
|
|||
(props || []).forEach(prop => { | |||
Object.values(props || {}).forEach(prop => { |
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.
declaredPropTypes
used to be an array here, but an object in prop-types
. Common implementation uses an object (keyed by prop name) for easier lookups
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 should use https://npmjs.com/object.values for back compat with older nodes.
} | ||
|
||
function handleCustomValidators(component) { | ||
const propTypes = component.declaredPropTypes; |
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 piece of code deals with used propTypes, but it was nested inside of markPropTypesAsDeclared
(which was extracted out).
lib/util/propTypes.js
Outdated
const classExpressions = []; | ||
const defaults = {skipShapeProps: true, customValidators: []}; | ||
const configuration = Object.assign({}, defaults, context.options[0] || {}); | ||
const customValidators = configuration.customValidators; |
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.
These two config options come from two different rules. Put them here for now, maybe will need to consider if all rules using this functionality should take both of these as options?
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.
is there a reason the options would differ across rules? if not, maybe it makes more sense to put these in "settings"?
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.
Actually I was partly wrong, customValidators
is present in both prop-types
and no-unused-prop-types
. skipShapeProps
comes from no-unused-prop-types
only, because it can be hard to figure out that a prop access corresponds to a shape. Not sure it's needed in prop-types
, will take a look.
One problem right now is skipShapeProps
is true
by default in no-unused-prop-types
, whereas prop-types
never ignores shape props. Will try to fix that by always building declaredPropTypes for shapes and checking the option when validating props.
lib/rules/no-unused-prop-types.js
Outdated
@@ -893,7 +454,7 @@ module.exports = { | |||
return; | |||
} | |||
|
|||
(props || []).forEach(prop => { | |||
Object.values(props || {}).forEach(prop => { |
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 should use https://npmjs.com/object.values for back compat with older nodes.
lib/rules/no-unused-prop-types.js
Outdated
return; | ||
} | ||
|
||
Object.values(propTypes).forEach(propType => { |
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
lib/rules/no-unused-prop-types.js
Outdated
|
||
if (node && ( | ||
node.type === 'ArrowFunctionExpression' | ||
|| node.type === 'FunctionExpression' |
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.
it'd be really nice if eslint exposed some kind of "is function" helper :-/ but lacking that, should we have one ourselves?
lib/util/propTypes.js
Outdated
// Necessary because babel's scopes do not track type annotations. | ||
const classExpressions = []; | ||
const defaults = {skipShapeProps: true, customValidators: []}; | ||
const configuration = Object.assign({}, defaults, context.options[0] || {}); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lib/util/propTypes.js
Outdated
const classExpressions = []; | ||
const defaults = {skipShapeProps: true, customValidators: []}; | ||
const configuration = Object.assign({}, defaults, context.options[0] || {}); | ||
const customValidators = configuration.customValidators; |
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.
is there a reason the options would differ across rules? if not, maybe it makes more sense to put these in "settings"?
lib/util/propTypes.js
Outdated
const customValidators = configuration.customValidators; | ||
const skipShapeProps = configuration.skipShapeProps; | ||
const sourceCode = context.getSourceCode(); | ||
const propWrapperFunctions = new Set(context.settings.propWrapperFunctions || []); |
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.
new Set
takes null and undefined quietly, so i'm not sure the || []
is needed.
lib/util/propTypes.js
Outdated
* @returns {Object|ASTNode} Either the whole scope or the ASTNode associated with the given identifier. | ||
*/ | ||
function typeScope(key, value) { | ||
if (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.
is there another way to write this function so it doesn't use arguments
?
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.
without changing it too much, I could check for key
and/or value
to be undefined I guess
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.
an alternative is creating multiple functions rather than overloading this one?
lib/util/propTypes.js
Outdated
} | ||
seen.add(annotation); | ||
|
||
switch (annotation.type) { |
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.
not a huge deal, but it'd be great to avoid a switch statement
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.
hmm do you mean a map with handlers for each type? there's also a bunch of other huge switches in here (like in other parts of the codebase) which could also get the same treatment
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/else, or a map, yes
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.
Not sure how 4-5 chained else-ifs are better than a switch? anyway will try out a map
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.
they don't suffer from unintentional fallthrough.
@ljharb thanks for the review. I'm still changing things up here fixing tests, just wanted to make sure I'm not doing anything outlandish. |
@ljharb I responded to your comments. Could you also take a look at the items in the PR description to see if they make sense? If they do, should I continue modifying this PR, or would it get too big? |
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 in general your direction looks good; we should strive to separate as many changes as possible into separate PRs, land those, and then rebase this one - that way the size of the overall change won’t be a problem.
lib/rules/display-name.js
Outdated
@@ -116,7 +117,7 @@ module.exports = { | |||
); | |||
|
|||
const namedFunctionExpression = ( | |||
(node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') && | |||
astUtil.isFunctionLikeExpression(node) && |
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 this kind of change could be peeled off into a separate pr, and landed first?
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.
Ah, you’re right 👍
For those other changes though all of them depend on this one to be merged first. I already have a bunch of new tests for item 2, I can push those separately then if you’d like. I’ll split out the function helper, and then this PR can be considered complete. |
a1d9300
to
2cfaaba
Compare
Split that out into #1914 |
2cfaaba
to
50cd5a6
Compare
Rebased and the build is now passing. |
@ljharb by itself this PR won’t fix #1908 as it doesn’t touch that particular rule. |
@yannickcr @lencioni @EvHaus any chance you could take a look? asking because I have a truckload of sweet refactoring elsewhere that all depends on this one (see PR description) and I really like how it turned out. |
I'll merge tonight if no issues are raised. |
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.
Wow, thanks for doing this extraction. I was too scared to do it when I originally wrote the no-unused-prop-types
rule, but it's very much needed will make sure all rules follow the same prop detection logic.
I didn't see any major issues or questions, and given that all the unit tests still pass -- I have pretty good confidence in merging this in.
This still fails a bunch of tests, but wanted to put it up for preliminary high-level review early.The reason tests are failing is because I extracted similar code fromprop-types
andno-unused-prop-types
but those two have diverged and I need to reconcile two implementations into a single helper.Next steps:
prop-types
andno-unused-prop-types
? (PR in my fork: Extract used prop types detection code alexzherdev/eslint-plugin-react#2)require-default-props
anddefault-props-match-prop-types
also have some common functionality for requiredness and defaultProps detection - extract those? That would be a prerequisite to those rules using the common helper. (PRs in my fork: Extract defaultProps detection alexzherdev/eslint-plugin-react#3, Extract required propTypes detection alexzherdev/eslint-plugin-react#4)