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

Fix required prop types for classes with annotated methods. #1112

Merged

Conversation

ethanjgoldberg
Copy link
Contributor

The react/prop-types rule incorrectly considers flow type
annotations on class methods with arguments named props as
prop type declarations for the class, even though flow itself
gives them no such weight.

When a react component is defined as a class extending React.Component,
and it defines a method that takes an annotated argument named props,
then, even though flow still thinks the props have any type, the
prop-types rule will not produce 'missing in props validation' errors.

A pattern that gets used a lot in React components is as follows:

type Props = {...};
class Comp extends React.Component {
  constructor(props: Props) {
    super(props);
    // do something...
  }

  // ...
}

which makes this especially nefarious.

@ethanjgoldberg ethanjgoldberg force-pushed the method-annotations-are-not-prop-types branch 2 times, most recently from 7e66da6 to 5917ab5 Compare May 26, 2017 19:16
The `react/prop-types` rule incorrectly considers  flow type
annotations on class methods with arguments named `props` as
prop type declarations for the class, even though flow itself
gives them no such weight.

When a react component is defined as a class extending `React.Component`,
and it defines a method that takes an annotated argument named `props`,
then, even though flow still thinks the props have `any` type, the
`prop-types` rule will not produce 'missing in props validation' errors.

A pattern that gets used a lot in React components is as follows:
```javascript
type Props = {...};
class Comp extends React.Component {
  constructor(props: Props) {
    super(props);
    // do something...
  }

  // ...
}
```
which makes this especially nefarious.
@ethanjgoldberg ethanjgoldberg force-pushed the method-annotations-are-not-prop-types branch from 5917ab5 to 6e8f543 Compare May 26, 2017 19:18
@ethanjgoldberg
Copy link
Contributor Author

ethanjgoldberg commented May 26, 2017

I've rebased this to fix the merge conflicts and bring the branch up to date -- could we get it merged this time? :)

@ljharb ljharb merged commit dc03975 into jsx-eslint:master May 27, 2017
@ljharb
Copy link
Member

ljharb commented Jun 1, 2017

@yannickcr it'd be great to get a release out with this fix in it; any chance you'll have time soon?

@ethanjgoldberg
Copy link
Contributor Author

@ljharb, I think this one is actually fixed by the latest flow (first change listed here) -- might be irrelevant now or possibly even wrong. I haven't tested the new flow, but I suspect this should be reverted.

@ljharb
Copy link
Member

ljharb commented Jun 2, 2017

We need to support both versions of flow, so reverting it wouldn't work by itself

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

Successfully merging this pull request may close these issues.

None yet

3 participants