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

[vaniyokk]

Same problem

[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.