Add more support for wrapped propTypes #1272
Conversation
|
Thanks! I think it'd be better, rather than only adding this option to individual rules, to also add it to "settings" (like the React pragma). Basically, individual rule settings would take precedence, and lacking that, the global settings would be used. |
|
Ah, right after pushing this, I saw that you'd like to have it in settings and individual rule configuration. What case did you have in mind where it would be necessary to have the option to override it per-rule? |
dustinsoftware
changed the title
|
No use case specifically; just aiming for maximum configurability. Let's keep it how you have it for now, and we can add the specific-rule settings later if needed. |
| @@ -63,7 +56,7 @@ module.exports = { | |||
| var configuration = Object.assign({}, defaults, context.options[0] || {}); | |||
| var skipShapeProps = configuration.skipShapeProps; | |||
| var customValidators = configuration.customValidators || []; | |||
| var propWrapperFunctions = new Set(configuration.propWrapperFunctions || []); | |||
| var propWrapperFunctions = new Set(context.settings.propWrapperFunctions || []); | |||
There was a problem hiding this comment.
will context.settings always exist and be an object?
There was a problem hiding this comment.
Yes, I tested both with and without settings defined - context.settings will always be an object.
| if ( | ||
| node.type === 'CallExpression' && | ||
| propWrapperFunctions.has(node.callee.name) && | ||
| node.arguments && node.arguments[0] |
There was a problem hiding this comment.
this should probably be node.arguments.length > 0?
There was a problem hiding this comment.
It shouldn't matter, if arguments is empty, then arguments[0] will return undefined, which will cause this expression to evaluate false. It also handles the case where the first element of arguments contains something that is falsy :)
It looks like checking the arguments array this way also matches code style in other places in this project.
There was a problem hiding this comment.
in v8, accessing an arguments index that's greater than length deoptimizes, so it'd be a performance issue - altho in this case it's not an arguments object, so it's probably fine :-)
|
This needs to be merged before the next release, so if I don't hear any objections by Monday, I'm going to merge it. |
|
Pushed support for a couple more, I think that's all the major prop-type rules. |
|
I wonder if, like Components.detect, we could make a PropTypes.detect function that would make this logic reusable? |
|
I think that could be helpful, but maybe out of scope for this PR. There's a lot of logic around finding declared prop types that is duplicated across multiple rules. |
Fixes #1266