False positives of react/prop-types in forwardRefs with 7.27.0

[gunters63]

prop type checking seems to broken inside forwardRef with version 7.27.0, 7.26.1 still worked fine.

The error only happens for type intersection hierarchies (I didn't test interface extensions).

When you have types like this:

type ControlProps = {
  className?: string | undefined;
};

type NamedProps = {
  name: string;
};

type ControlPropsWithChildren = ControlProps & {
  children?: ReactNode;
};

type BaseButtonProps = ControlPropsWithChildren &
  NamedProps & {
    onClick?: (() => void) | undefined;
    onMouseDown?: (() => void) | undefined;
    onMouseUp?: (() => void) | undefined;
    disabled?: boolean | undefined;
    width?: number;
    type?: 'submit' | 'reset' | 'button' | undefined;
  };

The plugin will now falsely flag all properties in nested type intersections as missing in props validation, in this case name, className and children (see screenshot wiggly lines).

The problem does not happen when the component is not wrapped in forwardRef.
It also didn't happen with version 7.26.1

D:\...\BaseButton.tsx
  23:7  error  'name' is missing in props validation       react/prop-types
  24:7  error  'className' is missing in props validation  react/prop-types
  28:7  error  'children' is missing in props validation   react/prop-types

Its likely that this commit: 4bc3499 triggered the problem

[ljharb]

cc @vedadeepta

[mataspetrikas]

I have noticed a potential cause/fix.

The validation fails in the following case:

type InputTextProps = InputHTMLAttributes<HTMLInputElement> & {
  label: string;
  helperText?: string;
  error?: boolean;
};

export default forwardRef<HTMLInputElement, InputTextProps>(function InputText(
  props: InputTextProps,
  forwardedRef: Ref<HTMLInputElement>,
) {
  const { name, value, label, error = false, helperText, ...rest } = props;
  const [isFocused, setIsFocused] = useState<boolean>(false);
  const isActive = (typeof value === 'string' && value.length > 0) || isFocused;

  function handleFocus(event: FocusEvent<HTMLInputElement>) {
    setIsFocused(true);
    props.onFocus?.(event);
  }

  function handleBlur(event: FocusEvent<HTMLInputElement>) {
    if (!event.target.value) setIsFocused(false);
    props.onBlur?.(event);
  }

  function handleChange(event: ChangeEvent<HTMLInputElement>) {
    props.onChange?.(event);
  }

  return (
    <div className="relative">
      <input
        autoComplete="off"
        {...rest}
        value={value || ''}
        name={name}
        onChange={handleChange}
        onFocus={handleFocus}
        onBlur={handleBlur}
        ref={forwardedRef}
      />
      <label
        htmlFor={name}
      >
        {label}
      </label>
      {helperText && (
        <div>
          {helperText}
        </div>
      )}
    </div>
  );
});

but it's all good when we change the name of the parameter to something else than props:

export default forwardRef<HTMLInputElement, InputTextProps>(function InputText(
  componentProps: InputTextProps,
  forwardedRef: Ref<HTMLInputElement>,
) {
  const { name, value, label, error = false, helperText, ...rest } = componentProps;

Do we have a false positive that matches only the component props?

[gunters63]

Interesting.

On the other hand, the validation still fails when you destructure right away in the function parameters:

const BaseButton = forwardRef<HTMLButtonElement, BaseButtonProps>(
  (
    {
      name,
      className,
      onClick,
      onMouseDown,
      onMouseUp,
      children,
      disabled,
      width,
      type,
    }: BaseButtonProps,
    ref,
  ): JSX.Element => (

[nzdjb]

I've put together a reasonably minimal example that triggers this:

import React, { forwardRef } from 'react'

interface D {
  x: string
  y: string
  z: string
}
interface C extends 
  Omit<D, 'z'> {}
const A = forwardRef<HTMLDivElement, C>(({ x, y }, ref) => {
  return (
    <div ref={ref}>
      {x} {y}
    </div>
  )
})

Output:

/workspaces/redwood-type-lint-issue/index.tsx
  9:44  error  'x' is missing in props validation  react/prop-types
  9:47  error  'y' is missing in props validation  react/prop-types

✖ 2 problems (2 errors, 0 warnings)

Remove the omission and the props are validated correctly:

interface C extends 
  D {}

Issue seems to be that type is missing at https://github.com/yannickcr/eslint-plugin-react/blob/94826da35804b7ea1ae5adcd6ba4b8e049418cf1/lib/rules/prop-types.js#L107

[vedadeepta]

@gunters63 i'm not able to replicate.

I pulled the latest and put your code almost exactly in the test case and it passed, maybe some intermediate commit fixed it

Here is the sample test case

  valid: parsers.all([].concat(
    {

      code: `

        import React, { forwardRef } from 'react';
        type ControlProps = {
          className?: string | undefined;
        };

        type NamedProps = {
          name: string;
          test: number;
        };

        type ControlPropsWithChildren = ControlProps & {
          children?: ReactNode;
        };

        type ButtonProps = ControlPropsWithChildren &
          NamedProps & {
            onClick?: (() => void) | undefined;
            onMouseDown?: (() => void) | undefined;
            onMouseUp?: (() => void) | undefined;
            disabled?: boolean | undefined;
            width?: number;
            type?: 'submit' | 'reset' | 'button' | undefined;
        };

        const BaseButton = forwardRef<HTMLButtonElement, ButtonProps>((
          {
            name,
            className,
            onClick,
            onMouseDown,
            onMouseUp,
            children,
            disabled,
            width,
            type,
          },
          ref,
        ): JSX.Element => {
          return <span>{width}</span>
        })

      `,
      features: ['ts', 'no-babel'],

    },
  )),

  invalid: []

cc @ljharb

@nzdjb yes my PR didn't handle special TS types like Omit, Partial, Exclude etc. That will require a separate PR and I'm not sure about the effort. Not planning to do it in the next few weeks. Apologies.

[ljharb]

Ideally we wouldnt have to explicitly support such things; the TS parser should have the type information we need already in the AST, thereby handling every possible type.

[gunters63]

This starts to get mysterious :)

If I inline the type definitions like this:

type NamedProps = {
  name: string;
};

type ControlProps = {
  className?: string | undefined;
};

type ControlPropsWithChildren = ControlProps & {
  children?: ReactNode;
};

type BaseButtonProps = ControlPropsWithChildren &
  NamedProps & {
    onClick?: (() => void) | undefined;
    onMouseDown?: (() => void) | undefined;
    onMouseUp?: (() => void) | undefined;
    disabled?: boolean | undefined;
    width?: number;
    type?: 'submit' | 'reset' | 'button' | undefined;
  };

const BaseButton = forwardRef<HTMLButtonElement, BaseButtonProps>(
  (
    {
   ....

the error goes away.

But my code originally does this:

import { ControlPropsWithChildren, NamedProps } from '$/components/BaseControl';

type BaseButtonProps = ControlPropsWithChildren &
  NamedProps & {
    onClick?: (() => void) | undefined;
    onMouseDown?: (() => void) | undefined;
    onMouseUp?: (() => void) | undefined;
    disabled?: boolean | undefined;
    width?: number;
    type?: 'submit' | 'reset' | 'button' | undefined;
  };

const BaseButton = forwardRef<HTMLButtonElement, BaseButtonProps>(
  (
    {
   ....

Contents of $/components/BaseControl:

import { ReactNode } from 'react';

export type ControlProps = {
  className?: string | undefined;
};

export type NamedProps = {
  name: string;
};

export type ControlPropsWithChildren = ControlProps & {
  children?: ReactNode;
};

If I import the base types like the code initially does the error comes back.

[vedadeepta]

@gunters63 ahh i see, if i understand correctly, currently we cannot parse types coming from external files.

@ljharb any thoughts on how to handle types coming from other files?

[gunters63]

@vedadeepta:

I have many other components using those external types which do not use forwardRef. Those work fine.

[ljharb]

@vedadeepta the TS parser should provide all those types; we shouldn't have to figure them out ourselves.

[JameelKhan9]

Can also confirm exporting the type in the same file breaks only for forward refs

"@types/react": "^17.0.37",
"@typescript-eslint/eslint-plugin": "^5.6.0",
"@typescript-eslint/parser": "^5.6.0",

The following is with the export:

In saying this, I have no idea why exporting the type would break eslint from getting the proptype. 😕

seems similar to the discussion happening at #3015

[vedadeepta]

@ljharb we're not getting the imported types in the AST. I checked the AST by creating a sample repo and importing types from other files.

Also its not just that the imported types don't work with forwardRef<> but they also don't work in general. we just don't throw an error for other cases.

I don't think this case can be handled without using the TS compiler API which can get the types from other files.

[gunters63]

@vedadeepta: As long as this is not implemented, wouldn't it be better to remove the special handling of forwardRef introduced in commit 4bc3499?

[vedadeepta]

Maybe, I'm not sure. This does not work for other React generic like FC<> etc, the only difference is that it fails silently

import { ControlProps, NamedProps } from './props';

type ButtonProps = ControlProps &
      NamedProps & {
        text: string;
    };
const Person2: React.FC<ButtonProps> = (props) => (
  <span>{props.somethingNotDef}</span> // <-- react/prop-types Does not throw an error but it should,
);

So we will need to remove all ts handling since it cannot get types imported from other files

[ljharb]

In that case yes, we should remove it.

[ljharb]

Or at least, it should only warn when the entire type is defined in the same file?

[JDMathew]

An intermediate solution would be to wrap the FC export like so:

export interface IInputFieldProps extends InputHTMLAttributes<HTMLInputElement> {
...
}

const InputField = (props: IInputFieldProps, forwardedRef: Ref<HTMLInputElement>,) => {
return (
      <>
        <input {...props} ref={forwardedRef}></input>
      </>
    )
  });

export default forwardRef<HTMLInputElement, IInputFieldProps>(InputField);

[aakash-lohono]

hi,
another intermediate fix is to specify the argument types explicitly, like so

import React, { ForwardedRef, forwardRef } from "react";

export interface HelloWorldProps {
  subject: string;
  className?: string;
}

const HelloWorld = forwardRef(
  function HelloWorldBase (
    props: HelloWorldProps,
    ref: ForwardRef<HtmlDivElement>,
  ) {
    const { subject, className } = props;

    return (
      <div 
        className={className}
        ref={ref}
      >
        Hello {subject}
      </div>
    );
  },
);

export default HelloWorld;

[gunters63]

hi, another intermediate fix is to specify the argument types explicitly, like so

Nice find! This workaround is low-effort and works well, I finally can unpin the package version again :)

[ljharb]

@vedadeepta any chance you'd be able to work on a PR for this?

[mobily]

@ljharb I can take it if you and @vedadeepta don't mind :)

[ljharb]

That'd be great!

[vedadeepta]

thanks @mobily