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

New rule: no-invalid-default-props #1022

Closed
ljharb opened this issue Jan 13, 2017 · 11 comments
Closed

New rule: no-invalid-default-props #1022

ljharb opened this issue Jan 13, 2017 · 11 comments

Comments

@ljharb
Copy link
Member

ljharb commented Jan 13, 2017

I'd like a rule that errors if anything is in defaultProps that is not also in propTypes.

This came up with the following code:

Component.propTypes = {
  fooValue: PropTypes.string,
};
Component.defaultProps = {
  foo: '',
};

In other words, it was a partially completed prop rename.

This rule should also look up a propTypes variable (Component.propTypes = propTypes) defined elsewhere in the file.

@webOS101
Copy link
Contributor

webOS101 commented Feb 23, 2017

I've taken a stab at this and have a working prototype. Just trying to get all the tests update for this rule. If @ljharb or someone else can clarify: I'm basing off an existing rule but need to write different data into the component list. Do rules share the same component data or do they each get their own copies? I see some rules appear to share the same data properties but not sure if they're somehow avoiding stepping on each others toes or if it's just luck that it doesn't conflict.

@webOS101
Copy link
Contributor

webOS101 commented Feb 23, 2017

Also, I considered it a violation of the rule if supplying a default for an .isRequired property. Was that your intention or should that be configurable?

@ljharb
Copy link
Member Author

ljharb commented Feb 23, 2017

That's a good point.

I think that should be an option, but the checking be enabled by default. Let's call it allowRequiredDefaults, and default it to false.

@webOS101
Copy link
Contributor

webOS101 commented Feb 23, 2017

Also discovered a bug in require-default-props. Oops.

webOS101 added a commit to webOS101/eslint-plugin-react that referenced this issue Feb 23, 2017
webOS101 added a commit to webOS101/eslint-plugin-react that referenced this issue Apr 30, 2017
@sholladay
Copy link

sholladay commented Dec 1, 2017

I am trying to understand the intention behind the allowRequiredDefaults option and why it defaults to false. Can you provide some insight @webOS101?

Surely this code is okay?

Component.propTypes = {
  name : PropTypes.string.isRequired
};
Component.defaultProps = {
  name : 'John Doe'
};

@ljharb
Copy link
Member Author

ljharb commented Dec 1, 2017

@sholladay no, that code is not OK - since name is required, the defaultProp will never trigger, so it's redundant - and redundant code should be removed.

@sholladay
Copy link

sholladay commented Dec 1, 2017

That is incorrect. Default props are applied before type checking happens.

From Typechecking With PropTypes:

The propTypes typechecking happens after defaultProps are resolved, so typechecking will also apply to the defaultProps.

I also confirmed that this includes isRequired by playing around with my React app. Admittedly, React is still a bit new to me, so I could be missing something. But this is in line with what I would expect.

Outside of the React ecosystem, code like this is pretty normal...

const currentUsername = 'John Doe';  // something dynamic
const getUser = (username = currentUsername) {
    if (!username) {
        throw new Error('Username is required');
    }
    // return { ... }
}

I definitely think it's a good thing to mark something as required, even if it has a default.

@ljharb
Copy link
Member Author

ljharb commented Dec 1, 2017

If you pass undefined, defaultProps will be applied, but if you pass null, they will not be.

Conceptually though, if a prop is required, it must be passed - having the defaultProp creates a situation where you could safely omit a required prop, which is a bug. This linting rule helps prevent that bug.

@webOS101
Copy link
Contributor

webOS101 commented Dec 2, 2017

@ljharb is correct, marking a prop as required but using defaultProps to make it optional does not make sense. Either it is required or it is not.

@sholladay
Copy link

sholladay commented Dec 2, 2017

To each their own, I guess. Thanks for providing an option. I am using isRequired to say that it must have a value, not that the caller of the component must provide one. I'm happy to have a sane default (and to be able to use it) but maybe tomorrow that won't be the case and it will still need a value, so I'm glad to be able to specify these things separately. At the very least, it will make refactoring easier.

@ljharb
Copy link
Member Author

ljharb commented Dec 2, 2017

Adding a defaultProp ensures it has a value; propTypes are solely about what the user provides.

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

No branches or pull requests

3 participants