Skip to content

Flow + exact types + spread breaks no-unused-prop-types rule #2138

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

Closed
peterkhayes opened this issue Jan 18, 2019 · 6 comments · Fixed by #2446
Closed

Flow + exact types + spread breaks no-unused-prop-types rule #2138

peterkhayes opened this issue Jan 18, 2019 · 6 comments · Fixed by #2446

Comments

@peterkhayes
Copy link

peterkhayes commented Jan 18, 2019

I'm using Flow for a React project, and find the no-unused-prop-types rule very helpful. However, it seems to silently fail when combining two exact types to make a component's props. This situation commonly occurs when working with Redux. For example, spreading exact types for OwnProps, StateProps, and DispatchProps into a new exact type is the recommended practice for typing connected components; see this comment for more info.

Here's a simple example. Even though unusedProp is not used, no error is displayed:

// @flow
import React from 'react';

type UsedProps = {|
  usedProp: number,
|};

type UnusedProps = {|
  unusedProp: number,
|};

type Props = {| ...UsedProps, ...UnusedProps |};

function MyComponent({ usedProp }: Props) {
  return <div>{usedProp}</div>;
}

If I switch to the following, ESLint now reports the error, but Flow (correctly) complains that two exact types cannot be intersected.

type Props = UsedProps & UnusedProps

This error is preventing us from properly typing our connected React components with Flow; I'd be happy to work on it if someone could show me where to look. The code for extracting props from flow types is pretty dense.

@ljharb
Copy link
Member

ljharb commented Jan 18, 2019

cc @alexzherdev for some direction in the props code :-)

@alexzherdev
Copy link
Contributor

Yeah I don't think we're handling spreads like this.

It looks like, when we're iterating through properties of the ObjectTypeAnnotation here, the iterateProperties helper will see an ObjectTypeSpreadProperty like ...UsedProps here.

From there, I think you should be able to use the getInTypeScope function to resolve the name like "UsedProps" to the actual node that contains the definition of that type. You can try "yielding" the properties of that node to the callback passed to iterateProperties, but I'd also double-check that any duplicates are handled properly (i.e. if both UsedProps and UnusedProps types contain the same property).

@pbondoer
Copy link

pbondoer commented Jan 30, 2019

We are also experiencing this. I'm willing to take a look if nobody's worked on it yet. Any pointers on how I can easily test this?

@peterkhayes
Copy link
Author

I have not done anything yet here, so feel free.

moroine pushed a commit to moroine/eslint-plugin-react that referenced this issue Oct 5, 2019
@ljharb ljharb closed this as completed in 11dc56b Oct 16, 2019
@levenecav
Copy link

After this update, it stopped working for me. 7.16.0 - everything was fine, after 7.17.0 - below.

type PropsState = {|
    brokenExchangeConnection: boolean
|};

type PropsActions = {|
    updateUserParams: (string, StreamingUserParamsItem) => StreamingUpdateUserParamsAction
|};

type PropsParent = {|
    onTenorChange: (SettingsStreamerAccountsPair, StreamingUserParamsItem) => void
|};

type Props = {|
    ...PropsState,
    ...PropsActions,
    ...PropsParent
|};


51:31  error  'brokenExchangeConnection' PropType is defined but prop is never used  react/no-unused-prop-types
57:23  error  'updateUserParams' PropType is defined but prop is never used          react/no-unused-prop-types
70:20  error  'onTenorChange' PropType is defined but prop is never used             react/no-unused-prop-types

@ljharb
Copy link
Member

ljharb commented Jan 28, 2020

@levenecav please file a new issue.

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

Successfully merging a pull request may close this issue.

5 participants