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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

react/prop-types is missing in props validation on destructured prop types #2343

Closed
reintroducing opened this issue Jul 5, 2019 · 6 comments

Comments

@reintroducing
Copy link

I just recently upgraded to the latest version and started to see this. My component has proptypes that include

/** A notification to queue up for display. */
 notification: PropTypes.shape({...NotificationPropTypes}),

NotificationPropTypes are:

import PropTypes from 'prop-types';

export default {
    /** Additional class(es) to add to the component. */
    className: PropTypes.string,
    /** A unique ID to identify the notification with. The suggested way to create this is to use <a href="https://www.npmjs.com/package/uuid#quickstart---commonjs-recommended" target="_blank">uuid</a>. */
    id: PropTypes.string.isRequired,
    /** The message body of the notification. */
    message: PropTypes.node.isRequired,
    /** Determines the characteristics of the notification. */
    type: PropTypes.oneOf(['info', 'success', 'error', 'warning']),
    /** Whether to display the close button. */
    showClose: PropTypes.bool,
    /** The time (in milliseconds) that the notification stays open for. */
    duration: PropTypes.number,
    /** Whether the notification should close itself without having to click the close button after the specified duration. */
    autoClose: PropTypes.bool,
    /** A function to execute after the notification has been hidden. Will receive the notification's props. */
    onHidden: PropTypes.func
};

Offending code:

} else if (notification && !find(this._notificationQueue, {id: notification.id})) {

Its the notification.id part that its complaining about (id). Any reason why this would start to error now?

@ljharb
Copy link
Member

ljharb commented Jul 5, 2019

Are you saying it crashes?

Either way, i'd expect both the spread there to be unnecessary (just PropTypes.shape(NotificationPropTypes) works) but also that linting won't be able to follow statically to the definition of NotificationPropTypes in another module - so it can't know that those subproperties are propTyped.

@reintroducing
Copy link
Author

@ljharb You're right, the spread is unnecessary. It's not crashing, its giving an error and I had to add an eslint-disable to it. I'm fine with it not doing the static check because I understand that limitation, but I think it would be preferred that instead of erroring, it does nothing at all and logs nothing and just ignores it without having to eslint-disable it, like it seemed to do with previous versions prior to this update.

@ljharb
Copy link
Member

ljharb commented Jul 5, 2019

In this case tho, the linter can't know if your imported shape is correct, or if it's missing properties. The only way to be sure is to force a human to explicitly verify that - which is what the lint error does by forcing you to use an eslint override comment. What you're suggesting isn't a safe default, since it would silently ignore possibly incorrect patterns.

@reintroducing
Copy link
Author

@ljharb that's fair, and thanks for the explanation. I'm going to close this as a non-issue then. I am still curious though why this didn't error in prior versions? The code has been unchanged since we were probably on ESLint 2.x way back when. Were there updates made to this plugin to change that or do you think its an internal ESLint change that got smarter and started to cause this then? Appreciate the discussion as well.

@golopot
Copy link
Contributor

golopot commented Jul 6, 2019

I am still curious though why this didn't error in prior versions?

prop-types catches more cases since #2301.

@reintroducing
Copy link
Author

@golopot perfect, thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants