eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

react/prop-types & TypeScript: false positive with HTMLAttributes prop type

Open cebamps opened this issue 3 years ago • 3 comments

Using React.HTMLAttributes<HTMLButtonElement> as the prop type in a component definition causes false positives of react/prop-types.

Strangely enough, when adding indirection by extending that same type in an interface, the problem goes away. The same technique applied to a type alias for the same type does not work, however.

Even stranger, the last case in the minimal working example below shows a case where prop type T gives the false positive while T & T does not.

Version: [email protected]

Minimal working example:

import React from "react";

// ERROR: 'className' is missing in props validation  react/prop-types
const MyComponent = ({ className }: React.HTMLAttributes<HTMLButtonElement>) => null;

// ERROR: 'className' is missing in props validation  react/prop-types
type TypeAlias = React.HTMLAttributes<HTMLButtonElement>;
const MyComponent_Alias = ({ className }: TypeAlias) => null;

// ERROR: 'className' is missing in props validation  react/prop-types
interface ExtendedTypeAlias extends TypeAlias {}
const MyComponent_ExtendedAlias = ({ className }: ExtendedTypeAlias) => null;

// OK
interface ExtendedInterface extends React.HTMLAttributes<HTMLButtonElement> {}
const MyComponent_ExtendedInterface = ({ className }: ExtendedInterface) => null;

// OK !?
const MyComponent_AliasSelfIntersection = ({ className }: TypeAlias & TypeAlias) => null;

cebamps avatar Jul 08 '22 08:07 cebamps

That is indeed bizarre, I'd expect them all to work, or not work, the same.

Type resolution typically comes from the TS eslint resolver, so it's possible some of the issue is on that side (cc @bradzacher) but it'll need investigation here regardless.

ljharb avatar Jul 08 '22 16:07 ljharb

I'll start by saying that this rule seems like it is pretty redundant in TS code because TS will catch the case where you're using a property that's not declared in the props object type. I.e. the rule will double-report (and likely do a worse job than TS as it's not type-aware).


Type resolution typically comes from the TS eslint resolver

I don't think it does - it looks like the logic is hard coded to look for top-level type/interface declarations instead of using scope analysis: https://github.com/jsx-eslint/eslint-plugin-react/blob/5919660215f830780f0ab02349971cd68280ead9/lib/util/propTypes.js#L672 https://github.com/jsx-eslint/eslint-plugin-react/blob/5919660215f830780f0ab02349971cd68280ead9/lib/util/propTypes.js#L681-L684


There's a lot of code and utilities that your rules are built on top of - so it's hard to quickly decipher what the rule is doing specifically, but some quick poking around...

I can see that here you use the right-most name (i.e. HTMLAttributes): https://github.com/jsx-eslint/eslint-plugin-react/blob/5919660215f830780f0ab02349971cd68280ead9/lib/util/propTypes.js#L638-L640 to index this variable: https://github.com/jsx-eslint/eslint-plugin-react/blob/5919660215f830780f0ab02349971cd68280ead9/lib/util/propTypes.js#L104-L114 Which will return undefined, meaning the next line will return undefined: https://github.com/jsx-eslint/eslint-plugin-react/blob/5919660215f830780f0ab02349971cd68280ead9/lib/util/propTypes.js#L641-L642 and the traversal will completely exit: https://github.com/jsx-eslint/eslint-plugin-react/blob/5919660215f830780f0ab02349971cd68280ead9/lib/util/propTypes.js#L591-L592

So I think that what's happening is that the rule is just not registering any properties, nor is it marking the props as something the rule should ignore?

bradzacher avatar Jul 08 '22 16:07 bradzacher

Interesting, thanks, that's a really helpful analysis.

I also agree that if TS will catch things that aren't declared, then it might be better for the prop-types rule to merely ensure that there is a TS type (presumably not any, though).

ljharb avatar Jul 08 '22 16:07 ljharb