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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update forbid-prop-types: handle prop-type functions with no args #2163

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jack-lewin
Copy link

jack-lewin commented Feb 13, 2019

This PR aims to fix #1673 by adding a new option to the forbid-prop-types rule: empty.

This will catch prop types which are functions, such as shape() or arrayOf(), but are used with either no arguments, or empty arguments ({} or []). These prop types don't add a huge amount of value in those scenarios, and should be avoided if possible.

Feedback appreciated 馃檪

Show resolved Hide resolved lib/rules/forbid-prop-types.js
@@ -5,6 +5,8 @@

const astUtil = require('./ast');

const PROP_TYPE_FUNCTIONS = ['object', 'shape', 'instanceOf', 'oneOf', 'oneOfType', 'objectOf', 'arrayOf', 'exact'];

This comment has been minimized.

@jack-lewin

jack-lewin Feb 13, 2019

Author

I'm not sure whether there's a better way of checking this, instead of hard-coding the list?

This comment has been minimized.

@ljharb

ljharb Feb 14, 2019

Collaborator

something like

import PropTypes from 'prop-types';
Object.keys(PropTypes)

?

This comment has been minimized.

@jack-lewin

jack-lewin Feb 19, 2019

Author

That would include prop-types which are not functions, e.g. string.

Seeing as these are unlikely to change much, I think I'm happy with this, if you are?

This comment has been minimized.

@ljharb

ljharb Mar 5, 2019

Collaborator

True - but they do indeed change; I just added a new one a couple weeks ago (see #2188 where this technique actually prevented a bug)

Show resolved Hide resolved lib/rules/forbid-prop-types.js
@@ -5,6 +5,8 @@

const astUtil = require('./ast');

const PROP_TYPE_FUNCTIONS = ['object', 'shape', 'instanceOf', 'oneOf', 'oneOfType', 'objectOf', 'arrayOf', 'exact'];

This comment has been minimized.

@ljharb

ljharb Feb 14, 2019

Collaborator

something like

import PropTypes from 'prop-types';
Object.keys(PropTypes)

?

* @returns {Boolean} `true` if the PropType is a function, `false` if not.
*/
function isPropTypeAFunction(propType) {
return PROP_TYPE_FUNCTIONS.indexOf(propType) > -1;

This comment has been minimized.

@ljharb

ljharb Feb 14, 2019

Collaborator

i'm a bit confused here; are we already sure by the time propType comes in that it's off of prop-types? Or if i have import { shape } from 'airbnb-prop-types, will that trigger it?

This comment has been minimized.

@jack-lewin

jack-lewin Feb 14, 2019

Author

At this point propType can be from anywhere - I hadn't considered the scenario of named custom validators like airbnb-prop-types. So yes, the example you mentioned would also trigger this error.

I'm not sure how to go about avoiding that 馃槙 Do you have any ideas?

This comment has been minimized.

@ljharb

ljharb Feb 17, 2019

Collaborator

Track where it's imported/required from, and ensure it's 'prop-types'?

This comment has been minimized.

@jack-lewin

jack-lewin Feb 19, 2019

Author

I've given that a try 馃槃

Show resolved Hide resolved tests/lib/rules/forbid-prop-types.js Outdated

jack-lewin added some commits Feb 14, 2019

Only support official React prop-types
This tracks the import/require statements within a file, to ensure that
the prop-types we're checking actually come from the React prop-types
library.

This prevents the rule from being mistakenly triggered by imported
prop-types which have the same name as one of the React prop-types.
Show resolved Hide resolved lib/util/ast.js Outdated
@@ -67,6 +69,21 @@ module.exports = {
function isMissingArguments(type, value) {
const propIsAFunction = propsUtil.isPropTypeAFunction(type);

// disregard custom prop-type imports (i.e. not the react prop-types)

This comment has been minimized.

@ljharb

ljharb Feb 20, 2019

Collaborator

let's also be sure to support React.PropTypes

Show resolved Hide resolved tests/lib/rules/forbid-prop-types.js Outdated

jack-lewin and others added some commits Feb 20, 2019

Fix checks for deeply-nested objects
This ensures we can check prop-types when the definition is nested more
than one level deep. For example, `React.PropTypes.shape` will now be
checked. Previously, only a single level of nesting was supported.
Remove additional newline
Co-Authored-By: jack-lewin <newsletters+github@jacklewin.com>
@jack-lewin

This comment has been minimized.

Copy link
Author

jack-lewin commented Feb 20, 2019

I've added a lot of tests in this PR. Happy to cut them down if you think that would make it easier for other contributors to follow.

jack-lewin added some commits Mar 1, 2019

Make arguments check backwards-compatible
Previous versions of ESLint did not include the `parent` node property
for an AST node. This was fixed in v5.0.0-alpha.0

This commit ensures that the `forbidEmpty` rule is compatible versions
3 and 4 of ESLint.
@jack-lewin

This comment has been minimized.

Copy link
Author

jack-lewin commented Mar 1, 2019

Just made a couple of small changes to make sure this is backwards-compatible 馃檪

@@ -97,6 +139,7 @@ module.exports = {
value.callee.type === 'MemberExpression'
) {
value = value.callee;
value.parent = {arguments: declaration.value.arguments};

This comment has been minimized.

@ljharb

ljharb Mar 5, 2019

Collaborator

is this mutation necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.