Skip to content

[Bug]: forwardRef error: 'className' is missing in props #3684

@viveleroi

Description

@viveleroi

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
    My issue appears in the command-line and not only in the text editor

Description Overview

The following code from shadcn/ui triggers a lint error: 'className' is missing in props validationeslint[react/prop-types), except this it definitely defined in the prop types.

eslint-config-next 14.1.0
└── eslint-plugin-react 7.33.2

const FormItem = React.forwardRef<HTMLDivElement, React.HTMLAttributes<HTMLDivElement>>(
  ({ className, ...props }, ref) => {
    const id = React.useId()

    return (
      <FormItemContext.Provider value={{ id }}>
        <div className={cn('space-y-2', className)} ref={ref} {...props} />
      </FormItemContext.Provider>
    )
  }
)
FormItem.displayName = 'FormItem'

Expected Behavior

eslint-plugin-react version

7.33.2

eslint version

8.56.0

node version

v21.5.0

Activity

ljharb

ljharb commented on Jan 31, 2024

@ljharb
Member

I think this may be fixed by #3679 once it lands, and is a duplicate of #3521?

viveleroi

viveleroi commented on Jan 31, 2024

@viveleroi
Author

Possibly, but it seems like those involve nested forwardRef calls and mine doesn't. The only existing issue I had found was closed as fixed a long time ago but I'm on the latest version.

developer-bandi

developer-bandi commented on Feb 1, 2024

@developer-bandi
Contributor

in prop-types test code, below code is passed already

{
  code: `
      import React, { forwardRef } from "react";

      export interface IProps {
        children: React.ReactNode;
        type: "submit" | "button"
      }

      export const FancyButton = forwardRef<HTMLButtonElement, IProps>((props, ref) => (
        <button ref={ref} className="MyClassName" type={props.type}>
          {props.children}
        </button>
      ));
    `,
  features: ['ts', 'no-babel'],
}

so In prop-types, the forwardRef single use(not with memo) is not a problem

I think React.HTMLAttributes type inference error is cause of issue's error, so this issue is same to #3325

viveleroi

viveleroi commented on Feb 1, 2024

@viveleroi
Author

Except in another project we have HTMLAttributes and we get no errors. I'm not sure what the difference is (it being function vs arrow function didn't change anything). In both cases VSCode/TS properly says the type of className is string | undefined

export const BaseButton = forwardRef<HTMLButtonElement, PropsWithChildren<ButtonHTMLAttributes<HTMLButtonElement>>>(
  function baseButton({ className, pressed, uiVariant, ...props }, ref) {
    const classNames = clsx(className, styles.button, uiVariant && styles[uiVariant], {
      [styles.pressed]: pressed
    })

    return <button className={classNames} ref={ref} {...props} />
  }
)
developer-bandi

developer-bandi commented on Feb 2, 2024

@developer-bandi
Contributor

i think the diffrence is PropsWithChildren. To make it more general, the difference arises from importing types from different files.

Let me give you an example. If we expand the code you posted by including the import path, it will look like this:

import React, { forwardRef, PropsWithChildren } from 'react'

export const BaseButton = forwardRef<HTMLButtonElement, PropsWithChildren<ButtonHTMLAttributes<HTMLButtonElement>>>(
  function baseButton({ className, pressed, uiVariant, ...props }, ref) {
    const classNames = clsx(className, styles.button, uiVariant && styles[uiVariant], {
      [styles.pressed]: pressed
    })

    return <button className={classNames} ref={ref} {...props} />
  }
)

and in the code, props-type check is not performed and same thing happens with similar code below

// type.ts
interface PropsTypes{
  falsyTest:string
}

// test.js
import React from "react"
import {PropsTypes} from "./types"

export const App = (props: PropsTypes) => {
    console.log(props.test) // ?? is Ok
     return <div></div>;
}

We can roughly understand why this phenomenon occurs by looking at the rule code of props-type.

The components given as examples above are components.list(); It is returned to the component through , and at this time, if a false value is received from the mustBeValidated function, the subsequent rules are passed.

return {
'Program:exit'() {
const list = components.list();
// Report undeclared proptypes for all classes
values(list)
.filter((component) => mustBeValidated(component))
.forEach((component) => {
reportUndeclaredPropTypes(component);
});
},
};
}),
};

The value to look at in the mustBeValidated function below is ignorePropsValidation, which is defined as true and returns false when the type declaration is imported and loaded as in the example above.

function mustBeValidated(component) {
const isSkippedByConfig = skipUndeclared && typeof component.declaredPropTypes === 'undefined';
return Boolean(
component
&& component.usedPropTypes
&& !component.ignorePropsValidation
&& !isSkippedByConfig
);
}

For this reason, it appears that no error occurs in the example code.
I hope it will be of help. If you see anything wrong or have any questions, please leave a comment.

rbondoc96

rbondoc96 commented on Nov 28, 2024

@rbondoc96
Contributor

I ran into this issue on an updated version, but in a different manner. I was able to find a workaround, but I wanted to post my findings here in case it's of any use:

Versions (from package-lock.json)

* @types/react: 18.3.12
* eslint: 9.14.0
* eslint-plugin-react: 7.37.2
* typescript: 5.6.3

I replicated the type structure in the initial post and saw I wasn't getting the error (which is good):

import * as React from 'react';

export const SheetHeader = React.forwardRef<HTMLDivElement, React.HTMLAttributes<HTMLDivElement>>(
    // No lint error on `className`
    ({ children, className, ...props }, ref) => {
        return (
            <div className={cn('flex flex-col gap-2', 'text-center', 'sm:text-left', className)} ref={ref} {...props}>
                {children}
            </div>
        );
    },
);
SheetHeader.displayName = 'SheetHeader';

Explanation

I was doing some refactoring and was initially importing types from React by name. With this approach, I wasn't getting the linting error:

import { type ComponentPropsWithoutRef, forwardRef } from 'react';

export const SheetHeader = forwardRef<HTMLDivElement, ComponentPropsWithoutRef<'div'>>(
    // No lint error on `className`
    ({ children, className, ...props }, ref) => {
        return (
            <div className={cn('flex flex-col gap-2 text-center sm:text-left', className)} ref={ref} {...props}>
                {children}
            </div>
        );
    },
);
SheetHeader.displayName = 'SheetHeader';

But once I changed my code to use a namespace import to shorten my import code, I got the linting error:

// Same if I were to use `import React from 'react'`
import * as React from 'react';

export const SheetHeader = React.forwardRef<HTMLDivElement, React.ComponentPropsWithoutRef<'div'>>(
    // Lint error with `className` :(
    ({ children, className, ...props }, ref) => {
        return (
            <div className={cn('flex flex-col gap-2 text-center sm:text-left', className)} ref={ref} {...props}>
                {children}
            </div>
        );
    },
);
SheetHeader.displayName = 'SheetHeader';

Workaround

I was able to get around this by instead using React.JSX.IntrinsicElements['div'] instead of React.ComponentPropsWithoutRef<'div'> since it effectively works the same way:

  • ComponentPropsWithoutRef<T> uses PropsWithoutRef<ComponentProps<T>>...
    • and ComponentProps<T> uses JSX.IntrinsicElements[T] if T extends keyof JSX.IntrinsicElements
  • forwardRef<T, P> uses PropsWithoutRef<P> on the render callback, so the ref prop on P isn't actually used.
import * as React from 'react';

export const SheetHeader = React.forwardRef<HTMLDivElement, React.JSX.IntrinsicElements['div']>(
    ({ children, className, ...props }, ref) => {
        return (
            <div className={cn('flex flex-col gap-2 text-center sm:text-left', className)} ref={ref} {...props}>
                {children}
            </div>
        );
    },
);
SheetHeader.displayName = 'SheetHeader';

Important

However, this only solves forward ref components that use HTML element props.

Forward ref components that use another component's ref and rely on ComponentProps or ComponentPropsWithoutRef to extract that component's prop type don't have a nice workaround. Unless that component has a type alias for their props, like below:

import * as SheetPrimitive from '@radix-ui/react-dialog';
import * as React from 'react';

// Radix thankfully exports the prop type
export const SheetDescription = React.forwardRef<
    React.ElementRef<typeof SheetPrimitive.Description>,
    SheetPrimitive.DialogDescriptionProps
>(({ children, className, ...props }, ref) => {
    return (
        <DialogPrimitive.Description className={cn('text-sm text-muted-foreground', className)} ref={ref} {...props}>
            {children}
        </DialogPrimitive.Description>
    );
});
SheetDescription.displayName = 'SheetDescription';
ljharb

ljharb commented on Nov 29, 2024

@ljharb
Member

import * should be avoided in code regardless, but it would be ideal if using it didn't make the linter rule stop working.

rbondoc96

rbondoc96 commented on Nov 30, 2024

@rbondoc96
Contributor

import * should be avoided in code regardless, but it would be ideal if using it didn't make the linter rule stop working.

I usually try to avoid using import * in code, but after adopting ShadCN in my projects, they use it practically everywhere. It makes editing the copy-pasted code a bit tedious 😞 So I've chosen to ignore it for now.

I've been learning a bit more about how tree-shaking works, but can't say I'm too familiar with it yet. From what I've gathered so far, it seems like using the namespace import pattern isn't a huge issue when used on packages that support ESM along with bundlers like Vite?

ljharb

ljharb commented on Nov 30, 2024

@ljharb
Member

It's always an issue; tree-shaking can never do anywhere near as good as a job as only importing what you need in the first place. Simply having barrel files in your application will make your bundle and memory footprint much larger, no matter what optimizations you attempt to employ.

rbondoc96

rbondoc96 commented on Dec 1, 2024

@rbondoc96
Contributor

It's always an issue; tree-shaking can never do anywhere near as good as a job as only importing what you need in the first place. Simply having barrel files in your application will make your bundle and memory footprint much larger, no matter what optimizations you attempt to employ.

I see, thank you! I appreciate the explanation 🙇🏽‍♂️ Personally, I think I'll choose to not care about it for now since it's just me working on my projects. And I just want to get something out the door and develop as fast as possible. But I'm glad to know this so I can just refactor it all at once in the future 🥴

I think it is slightly unfortunate though that a lot of people using ShadCN might not know this and will continue to use that pattern, since it's what's shown in docs and what will eventually get copy-pasted.

ljharb

ljharb commented on Dec 1, 2024

@ljharb
Member

Indeed, it'd be ideal if they improved their docs to avoid this bad practice.

This is all a tangent tho, since we should still work with it :-)

rbondoc96

rbondoc96 commented on Dec 1, 2024

@rbondoc96
Contributor

I think I might have a possible fix. If you think it could be a viable solution, I could submit a PR with this.

Disclaimer: I'm not at all familiar with how ESLint rule packages work. This is my first time digging into it.


Findings

I found that searchDeclarationByName in propTypes.ts wasn't resolving an index from the node's type names:

Image

Possible Solution

After I added ComponentProps and ComponentPropsWithoutRef to allowedGenericTypes and genericTypeParamIndexWherePropsArePresent, I no longer got the linting errors when using the import * pattern:

Image

Image

ljharb

ljharb commented on Dec 1, 2024

@ljharb
Member

As long as it comes with test cases, that sounds great!

rbondoc96

rbondoc96 commented on Dec 3, 2024

@rbondoc96
Contributor

As long as it comes with test cases, that sounds great!

Cool 🙂 I submitted #3859.

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @ljharb@viveleroi@rbondoc96@developer-bandi

      Issue actions

        [Bug]: forwardRef error: 'className' is missing in props · Issue #3684 · jsx-eslint/eslint-plugin-react