-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Closed
Labels
Description
Is there an existing issue for this?
- I have searched the existing issues and my issue is uniqueMy issue appears in the command-line and not only in the text editorTo pick up a draggable item, press the space bar. While dragging, use the arrow keys to move the item. Press space again to drop the item in its new position, or press escape to cancel.
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 commentedon Jan 31, 2024
I think this may be fixed by #3679 once it lands, and is a duplicate of #3521?
viveleroi commentedon Jan 31, 2024
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 commentedon Feb 1, 2024
in prop-types test code, below code is passed already
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 commentedon Feb 1, 2024
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
isstring | undefined
developer-bandi commentedon Feb 2, 2024
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:
and in the code, props-type check is not performed and same thing happens with similar code below
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.
eslint-plugin-react/lib/rules/prop-types.js
Lines 189 to 201 in 9f4b2b9
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.
eslint-plugin-react/lib/rules/prop-types.js
Lines 78 to 86 in 9f4b2b9
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 commentedon Nov 28, 2024
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
)I replicated the type structure in the initial post and saw I wasn't getting the error (which is good):
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:
But once I changed my code to use a namespace import to shorten my import code, I got the linting error:
Workaround
I was able to get around this by instead using
React.JSX.IntrinsicElements['div']
instead ofReact.ComponentPropsWithoutRef<'div'>
since it effectively works the same way:ComponentPropsWithoutRef<T>
usesPropsWithoutRef<ComponentProps<T>>
...ComponentProps<T>
usesJSX.IntrinsicElements[T]
ifT extends keyof JSX.IntrinsicElements
forwardRef<T, P>
usesPropsWithoutRef<P>
on the render callback, so theref
prop onP
isn't actually used.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
orComponentPropsWithoutRef
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:ljharb commentedon Nov 29, 2024
import *
should be avoided in code regardless, but it would be ideal if using it didn't make the linter rule stop working.rbondoc96 commentedon Nov 30, 2024
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 commentedon Nov 30, 2024
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 commentedon Dec 1, 2024
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 commentedon Dec 1, 2024
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 commentedon Dec 1, 2024
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
inpropTypes.ts
wasn't resolving an index from the node's type names:Possible Solution
After I added
ComponentProps
andComponentPropsWithoutRef
toallowedGenericTypes
andgenericTypeParamIndexWherePropsArePresent
, I no longer got the linting errors when using theimport *
pattern:ljharb commentedon Dec 1, 2024
As long as it comes with test cases, that sounds great!
prop-types
: props missing in validation when using generic types from a namespace import #3859rbondoc96 commentedon Dec 3, 2024
Cool 🙂 I submitted #3859.