shadcn-ui / ui

Beautifully designed components that you can copy and paste into your apps. Accessible. Customizable. Open Source.
https://ui.shadcn.com
MIT License
74.12k stars 4.57k forks source link

Select: 'className' is missing in props validation #120

Closed marco-digennaro closed 2 months ago

marco-digennaro commented 1 year ago

In the Select component, the prop className doesn't exist in the interface so it gives the above error

Screenshot 2023-03-09 at 09 57 34

shadcn commented 1 year ago

Hmm that's odd. Trigger extends HTMLButtonElement so there should be a className in there.

marco-digennaro commented 1 year ago

it's actually happening also on other components

doneumark commented 1 year ago

I'm having the exact same problem :( also in Label, Select...

emicba commented 1 year ago

Hey @marco-digennaro @doneumark! It would be nice to have a minimal code example (maybe CodeSandbox?). I couldn't reproduce the error using TS 4.7.4

doneumark commented 1 year ago

Hey @emicba, It's actually the EXACT components above from the docs. But I think the problem is might be I have typescript 5? what happens if you update to the latest typescript 5 version? Thanks!

emicba commented 1 year ago

Seems to be working fine on TS 5.0.3.

litewarp commented 1 year ago

Was having trouble with this today until I removed the following line from my tsconfig.json:

preserveSymlinks: true
nickcoad commented 1 year ago

This is because of a change made in lucide-react I believe - using lucide-react@0.105.0 instead of the latest fixed this for me.

Edit: after some further investigation, the latest version you can use without issue is lucide-react@0.119.0. The next version is where the break occurs.

b0o commented 1 year ago

@nickcoad I tried downgrading to both lucide-react@0.119.0 and lucide-react@0.105.0, I'm still having the issue in the ContextMenu component. I even tried completely commenting out any Lucide icons and the Lucide import, I still have the issue.

nickcoad commented 1 year ago

@b0o interesting - what's an example of a specific line that is showing this error, can you paste it here?

b0o commented 1 year ago

2023-04-18_19-37-21_region

'use client'

import { DialogProps } from '@radix-ui/react-dialog'
import { Command as CommandPrimitive } from 'cmdk'
import { Search } from 'lucide-react'
import * as React from 'react'

import { cn } from '#/lib/utils'
import { Dialog, DialogContent } from '#/ui/shad/Dialog'

const Command = React.forwardRef<
  React.ElementRef<typeof CommandPrimitive>,
  React.ComponentPropsWithoutRef<typeof CommandPrimitive>
>(({ className, ...props }, ref) => (
  <CommandPrimitive
    ref={ref}
    className={cn('flex h-full w-full flex-col overflow-hidden rounded-lg bg-bl-gray-800', className)}
    {...props}
  />
))
Command.displayName = CommandPrimitive.displayName

That's just one example, but it occurs numerous times in that file.

3atharvkurdukar commented 1 year ago

The issue got resolved for me as soon as I turned on Next.js TS server instead of VSCode one.

landendanyluk commented 1 year ago

I'm having this issue inside the Dropdown Menu component as well as the Context Menu component. It doesn't seem as though any of the solutions above have fixed it for me.

SebastienWae commented 1 year ago

For the moment I have silenced that error by creating an interface that extend the props type. To use @b0o example:

interface Props extends React.ComponentPropsWithoutRef<typeof CommandPrimitive> {}
const Command = React.forwardRef<
  React.ElementRef<typeof CommandPrimitive>,
  Props
>(({ className, ...props }, ref) => (
  <CommandPrimitive
    ref={ref}
    className={cn('flex h-full w-full flex-col overflow-hidden rounded-lg bg-bl-gray-800', className)}
    {...props}
  />
))
Command.displayName = CommandPrimitive.displayName

You'll also have to add/update the following rule in your eslint config, or you'll gett the @typescript-eslint/no-empty-interface error by default.

"@typescript-eslint/no-empty-interface": [
      "error",
      {
        allowSingleExtends: true,
      },
    ],
DennohKim commented 1 year ago

Having the same problem while using the table component. Screenshot from 2023-06-05 01-35-50

billyjacoby commented 1 year ago

Having this same issue using the Tabs component

mzavattaro commented 1 year ago

@shadcn any update on this error? Seems to be plaguing everyone in our team as well.

rtorcato commented 1 year ago

This fixes the error, but is not the best solution. Haven't had time to work look at the existing types.

type props = React.ElementRef<typeof AvatarPrimitive.Root> & { className?: String }
type props2 = React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Root> & { className?: String }

const Avatar = React.forwardRef<props, props2>(({ className, ...props }, ref) => (
  <AvatarPrimitive.Root ref={ref} className={cn('relative flex h-10 w-10 shrink-0 overflow-hidden rounded-full', className)} {...props} />
))
Avatar.displayName = AvatarPrimitive.Root.displayName
shadcn commented 1 year ago

@billyjacoby @mzavattaro I still cannot reproduce this? Can you share some code I can take a look at?

billyjacoby commented 1 year ago

@shadcn Sure! It happens in both of the files in my browser app here:

https://github.com/billyjacoby/browsernaut/tree/main/src/components/ui

rtorcato commented 1 year ago

@billyjacoby @mzavattaro I still cannot reproduce this? Can you share some code I can take a look at?

disabling eslint-plugin-react in my eslintrc file makes the error go away.

https://www.npmjs.com/package/eslint-plugin-react

in shadcn repo the eslint rules are:

"extends": [ "next/core-web-vitals", "turbo", "prettier", "plugin:tailwindcss/recommended" ],

but they really should have some linting for react and that's why devs are having problems when they have
"plugin:react/recommended" in their project.

in my previous post this is the fix. For Avatar as an example.

const Avatar = React.forwardRef<
  React.ElementRef<typeof AvatarPrimitive.Root> & { className: String },
  React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Root> & {
    className: String
  }
>(({ className, ...props }, ref) => (
  <AvatarPrimitive.Root
    ref={ref}
    className={cn(
      "relative flex h-10 w-10 shrink-0 overflow-hidden rounded-full",
      className
    )}
    {...props}
  />
))

but there should be a more elegant way to define the props using generics.

Chuhj commented 1 year ago

https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/prop-types.md#as-for-exceptions It says

It would seem that some common properties such as props.children or props.className (and alike) need to be treated as exceptions.

So instead of disabling eslint-plugin-react, i think it is better way to ignore "className" prop in rules of eslintrc.

"rules": {
    "react/prop-types": [2, { "ignore": ["className"] }],
    ...
}

Also, it will not trigger an error in other components if the 'className' is missing from props. However, if you're using TypeScript, the compiler will throw an error, so you should be fine.

marlonmarcello commented 11 months ago

@Chuhj is best solution for now. Here is a link to the source of problem https://github.com/jsx-eslint/eslint-plugin-react/issues/3284

mimccio commented 11 months ago

To add to @Chuhj solution, I've used "overrides" to disable rule only for shadcn/ui components

  overrides: [
    {
      files: ['**/components/ui/*.tsx'], 
      rules: {
        'react/prop-types': [2, { ignore: ['className'] }],
        'react-refresh/only-export-components': 'off',
      },
    },
  ],
roger-tbg commented 11 months ago

@mimccio's solution is working great for me as a slight +1 to scope down @Chuhj's also great solution

stsiarzhanau commented 10 months ago

In my case, ESLint barks not only on className props, but on the other destructured props...

image

Despite the fact, that the type is inferred correctly...

image

For this given component I solved it by replacing React.ComponentPropsWithoutRef<typeof SeparatorPrimitive.Root> type annotation with the direct type import from the radix-ui lib.

image

image

Such approach seems quite logical when looking in the Separator source code. But I haven't looked into another components yet, so don't know whether it can be applied to them.

Update: I've looked briefly at some other components. And it seems the approach can be applied to them also. For example, for the Select component in question we can:

import type {
  SelectProps,
  SelectTriggerProps,
  SelectValueProps,
  SelectIconProps,
  SelectPortalProps,
  SelectContentProps,
  SelectViewportProps,
  SelectGroupProps,
  SelectLabelProps,
  SelectItemProps,
  SelectItemTextProps,
  SelectItemIndicatorProps,
  SelectScrollUpButtonProps,
  SelectScrollDownButtonProps,
  SelectSeparatorProps,
  SelectArrowProps,
} from '@radix-ui/react-select';

And then replace React.ComponentPropsWithoutRef<typeof SheetPrimitive.Content> with just SelectTriggerProps. And do the same for other subcomponets.

leimantas commented 10 months ago

Copilot chat answer:

If you're using ESLint with a TypeScript project, you might want to disable the react/prop-types rule, because it's not necessary when TypeScript is doing the type checking. You can do this in your ESLint configuration file (.eslintrc or .eslintrc.js) like this:

{
  "rules": {
    "react/prop-types": "off"
  }
}
rajeshdavidbabu commented 10 months ago

Alright. The culprit is eslintrc.cjs file.

I replaced mine with the Remix's eslint config and the error was gone. If you are using Next.js, then use the official eslintrc.cjs file from Next.js project.

My fix was this for a Remix project

inside .eslintrc.cjs

/** @type {import('eslint').Linter.Config} */
module.exports = {
  extends: ["@remix-run/eslint-config", "@remix-run/eslint-config/node"],
};
drake-nathan commented 9 months ago

Copilot chat answer:

If you're using ESLint with a TypeScript project, you might want to disable the react/prop-types rule, because it's not necessary when TypeScript is doing the type checking. You can do this in your ESLint configuration file (.eslintrc or .eslintrc.js) like this:

{
  "rules": {
    "react/prop-types": "off"
  }
}

I 2nd this. The prop-types rule doesn't offer anything in TS projects. TS itself will tell you when you're trying to use a prop that wasn't defined.

Sylpherena commented 8 months ago

You may have this problem with other props too, so in addition to @Chuhj 's answer, ESLint configuration (.eslintrc) files are hierarchical. So you can add one to your /ui directory and only your ui files will ignore this error. Solving like this made me feel better maybe someone else will want to solve like this

MrPossumz commented 5 months ago

For anyone else who stumbles upon this, the issue linked by @marlonmarcello has some interesting recent findings. Specifically that if you import HTMLAttributes directly from react, the error disappears. See https://github.com/jsx-eslint/eslint-plugin-react/issues/3284#issuecomment-2021754931

Quoting the reply here for convenience:

If you import directly from React the error message goes away.

Works:

import type { HTMLAttributes } from "react"
import { cn } from "../../lib/utils"

export function Heading({ className, ...props }: HTMLAttributes<HTMLDivElement>) {
  return <div className={cn("font-semibold text-lg", className)} {...props} />
}

Does not work:

import React from "react"
import { cn } from "../../lib/utils"

export function Heading({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) {
  return <div className={cn("font-semibold text-lg", className)} {...props} />
}
phil-hudson commented 4 months ago

For anyone else who stumbles upon this, the issue linked by @marlonmarcello has some interesting recent findings. Specifically that if you import HTMLAttributes directly from react, the error disappears. See jsx-eslint/eslint-plugin-react#3284 (comment)

Quoting the reply here for convenience:

If you import directly from React the error message goes away. Works:

import type { HTMLAttributes } from "react"
import { cn } from "../../lib/utils"

export function Heading({ className, ...props }: HTMLAttributes<HTMLDivElement>) {
  return <div className={cn("font-semibold text-lg", className)} {...props} />
}

Does not work:

import React from "react"
import { cn } from "../../lib/utils"

export function Heading({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) {
  return <div className={cn("font-semibold text-lg", className)} {...props} />
}

This works for me - is there an easy way that we can force this behavior for shadcn components?

borontion commented 4 months ago

For anyone else who stumbles upon this, the issue linked by @marlonmarcello has some interesting recent findings. Specifically that if you import HTMLAttributes directly from react, the error disappears. See jsx-eslint/eslint-plugin-react#3284 (comment)

Quoting the reply here for convenience:

If you import directly from React the error message goes away. Works:

import type { HTMLAttributes } from "react"
import { cn } from "../../lib/utils"

export function Heading({ className, ...props }: HTMLAttributes<HTMLDivElement>) {
  return <div className={cn("font-semibold text-lg", className)} {...props} />
}

Does not work:

import React from "react"
import { cn } from "../../lib/utils"

export function Heading({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) {
  return <div className={cn("font-semibold text-lg", className)} {...props} />
}

this also applies if you import forwardRef directly from react instead of using import * as React from "react"; ... React.forwardRef

borontion commented 3 months ago

I think the root cause comes from eslint-plugin-react. As mentioned in https://github.com/jsx-eslint/eslint-plugin-react/issues/3325#issuecomment-1179176878, type resolution for eslint does not come from ts lint resolve, instead, it is hard coded, for example this part for generic type params: https://github.com/jsx-eslint/eslint-plugin-react/blob/6ce58e52cda582171522bb27279b3329c22ae800/lib/util/propTypes.js#L110-L120

which does not include ComponentPropsWithoutRef. If you want to hack eslint, include ComponentPropsWithoutRef in this object, the issue will be gone (same for HTMLAttributes

There are several workarounds I can see:

  1. Directly use the props type instead of ComponentPropsWithoutRef:
const SelectTrigger = React.forwardRef<
  React.ElementRef<typeof SelectPrimitive.Trigger>,
  // React.ComponentPropsWithoutRef<typeof SelectPrimitive.Trigger>
  SelectPrimitive.SelectTriggerProps
>
  1. Change how react imported. This can bypass import check logic: https://github.com/jsx-eslint/eslint-plugin-react/blob/6ce58e52cda582171522bb27279b3329c22ae800/lib/util/propTypes.js#L641
// import React from "react";
// import * as React from "react";
import { ... } from "react";

Honestly I prefer to disable this rule, as I felt it should be covered with type check.

wan54 commented 3 months ago

For those who prefer not disabling the lint rule. The workaround until fixed in the source:

type CN<T> = T & { className: string };

const AccordionItem = React.forwardRef<
  React.ElementRef<typeof AccordionPrimitive.Item>,
  CN<React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Item>>
>(({ className, ...props }, ref) => (
  <AccordionPrimitive.Item ref={ref} className={cn('border-b', className)} {...props} />
));
SalahAdDin commented 3 months ago

For those who prefer not disabling the lint rule. The workaround until fixed in the source:

type CN<T> = T & { className: string };

const AccordionItem = React.forwardRef<
  React.ElementRef<typeof AccordionPrimitive.Item>,
  CN<React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Item>>
>(({ className, ...props }, ref) => (
  <AccordionPrimitive.Item ref={ref} className={cn('border-b', className)} {...props} />
));

Yeah, It works as a work around.

LuzAramburo commented 3 months ago

I have the same problem. This one was my workaround

https://github.com/shadcn-ui/ui/issues/120#issuecomment-1828081539

To add to @Chuhj solution, I've used "overrides" to disable rule only for shadcn/ui components

  overrides: [
    {
      files: ['**/components/ui/*.tsx'], 
      rules: {
        'react/prop-types': [2, { ignore: ['className'] }],
        'react-refresh/only-export-components': 'off',
      },
    },
  ],
shadcn commented 2 months ago

This issue has been automatically closed because it received no activity for a while. If you think it was closed by accident, please leave a comment. Thank you.

acharyarupak391 commented 1 month ago

@marco-digennaro

In my case, I just did & { className?: string } to manually add the className property to the existing type.

or in your case u could do React.ComponentPropsWithoutRef<typeof SelectPrimitive.Trigger> & { className?: string }

SalahAdDin commented 1 month ago

This issue has been automatically closed because it received no activity for a while. If you think it was closed by accident, please leave a comment. Thank you.

It hasn't been yet solved!

denlahodnyi commented 1 month ago

This is what helped me:

  1. Update eslint-plugin-react to be at least 7.36.0 (they claimed to fix it in that version - source)
  2. Import forwardRef directly (import { forwardRef } from 'react')

In that case, you don’t need to disable this eslint rule or add types manually.