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: require explicitly typed defaultProps #169

Closed
andrewbranch opened this issue Jun 23, 2018 · 5 comments
Closed

New rule: require explicitly typed defaultProps #169

andrewbranch opened this issue Jun 23, 2018 · 5 comments

Comments

@andrewbranch
Copy link

It's easy write a component with unsafe defaultProps:

interface MyComponentProps {
  onClick?: React.MouseEventHandler<HTMLElement>;
}

class MyComponent extends React.Component<MyComponentProps> {
  static defaultProps = {
    onClick: 42
  };

  render() {
    return <div onClick={this.props.onClick} />;
  }
}

So it's useful to enforce declaring an explicit type for static defaultProps (which should basically always be Partial<P> where P is the type of the component’s props), which would catch such silly mistakes:

interface MyComponentProps {
  onClick?: React.MouseEventHandler<HTMLElement>;
}

class MyComponent extends React.Component<MyComponentProps> {
  static defaultProps: Partial<MyComponentProps> = {
  //     ^^^^^^^^^^^^ Type { onClick: number } is not convertible to Partial<MyComponentProps> or some error message along these lines
    onClick: 42
  };
}
@suchanlee
Copy link
Contributor

I think it would be useful and would be happy to accept a PR!

@andrewbranch
Copy link
Author

andrewbranch commented Jul 5, 2018

I think to do this right, we’d need to use the type checker (ts.createProgram) to tell whether or not a class declaring defaultProps is in fact a React component. However, I'm not sure what the overhead for that is, and if it would be worth it just to check the AST for the text React.Component and maybe Component, also checking that Component is being imported from 'react' at some point (presumably faster but not as accurate). What are your feelings on it?

@andrewbranch
Copy link
Author

Actually, in TypeScript 3.0, you don’t want to explicitly type defaultProps. So, maybe instead of requiring an explicit type, a rule that checks that the inferred type of defaultProps is assignable to the class’s props interface would be valuable—which would definitely require the type checker.

@cheeZery
Copy link
Contributor

Also see this discussion (which unfortunately currently seems to be on hold): DefinitelyTyped/DefinitelyTyped#28515

@adidahiya
Copy link
Contributor

a rule that checks that the inferred type of defaultProps is assignable to the class’s props interface

this sounds like a good idea, but it should now be an eslint rule. see #210

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

No branches or pull requests

4 participants