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/no-unused-prop-types false positive if any one component doesn't use a field #2849

Closed
thehereward opened this issue Nov 2, 2020 · 4 comments · Fixed by #2852
Closed

Comments

@thehereward
Copy link
Contributor

thehereward commented Nov 2, 2020

In the following example both name and placeholder are triggering the no-unused-prop-types rule, despite both being used in separate components. I think the rule is triggering for name because it is unused in Comp1 and placeholder because it is unused in Comp2.

import React from "react";

type Props = {
  name: string;
  placeholder: string;
};

export const Comp1 = (props: Props): React.ReactElement => {
  const { placeholder } = props;
  return <div placeholder={placeholder} />
}

export const Comp2 = (props: Props): React.ReactElement => {
  const { name } = props;
  return <div>{name}</div>;
}

I appreciate that this example is contrived, but I think is the most basic reproduction case of a more complicated manifestation of the issue. The behaviour is kind of logical but not exactly intuitive.

@ljharb
Copy link
Member

ljharb commented Nov 2, 2020

This seems like a correct warning to me. Reusing the type in both components means both props are required in both of them. In this case, you shouldn't be sharing the Props type at all, since clearly the two components' types do not overlap.

@thehereward
Copy link
Contributor Author

thehereward commented Nov 3, 2020

Thank you for the reply (and adding the labels - apologies for not doing so).

I think I can be persuaded that the warning is indicative of a code smell in most cases. I needed to keep Props in my use-case so the solution I've gone with a solution that looks a bit like this:

type HasName = {
  name: string;
}
type HasPlaceholder = {
  placeholder: string;
}

type Props = HasName & HasPlaceholder;

The reason I thought it was counter-intuitive is that the linter was telling me "this prop is unused", when I could see that it clearly was being used.

Would you be happy with me raising a PR to add my contrived code as an example of incorrect code for this rule?

@ljharb
Copy link
Member

ljharb commented Nov 3, 2020

Absolutely, that’d be helpful!

I’d still love to understand your actual use case tho - not why you need the union type, but why the components need to use the overly broad union instead of the more granular type that matches their respective props.

@thehereward
Copy link
Contributor Author

PR is up.

To be clear, in my use case I don't think the components actually need to use the overly broad union; I've refactored to avoid this, which has resolved our linting error. One of the 'granular' types involved is a moderately complicated generic type from a third party library (react-final-form), which initially put me on the wrong path for identifying the cause of the linting error.

I am fairly confident that what we were doing was a bit of a code smell and the rule did help us spot the problem. Hopefully the addition to the docs will make it easier for the next person who encounters something similar.

@ljharb ljharb closed this as completed in 5708ecf Dec 18, 2020
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.

2 participants