-
Notifications
You must be signed in to change notification settings - Fork 990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(forms): Change typing to omit type
property + Workaround for upstream react/prop-types lint rule
#3762
Conversation
Hey @nzdjb nice find in the Link component! That should definitely be optionally typed. Also sorry if I sounded harsh in the original thread—all the work you did to upgrade ESLint was awesome and didn't go unnoticed! I'm not the best at TS so if I get something flat-out wrong please tell me. It looks to me like you're doubling up on work here. What I mean is that the extra object you're using to extend TextAreaFieldProps interface TextAreaFieldProps
extends Omit<FieldProps<HTMLTextAreaElement>, 'type'>,
Omit<React.ComponentPropsWithRef<'textarea'>, 'name'> {
name: string
id?: string
errorClassName?: string
errorStyle?: React.CSSProperties
validation?: RedwoodRegisterOptions
onBlur?: React.FocusEventHandler<Element>
onChange?: React.ChangeEventHandler<Element>
} FieldProps interface FieldProps<
Element extends
| HTMLTextAreaElement
| HTMLSelectElement
| HTMLInputElement = HTMLInputElement
> {
name: string
id?: string
errorClassName?: string
errorStyle?: React.CSSProperties
className?: string
style?: React.CSSProperties
validation?: RedwoodRegisterOptions
type?: string
onBlur?: React.FocusEventHandler<Element>
onChange?: React.ChangeEventHandler<Element>
} If the goal is to remove |
Hey @jtoar, no worries. I didn't read any harshness in your comments. :-) Credit for the fix in link goes to @callingmedic911 who spotted that I hadn't made it optional in the original PR (#3725 (review)). The change isn't directly related to this PR, but he suggested including it. I absolutely agree that this is doubling up and would like to avoid it. The issue is that the change from ESLint 7 to ESLint 8 (and accompanying eslint-plugin-react change) has caused it to start flagging the interfaces like TextAreaFieldProps as missing a bunch of fields when 'type' is omitted. This is documented in #3725 (comment). I see five possible approaches to solving this:
|
I think Tobbe has already merged a fix. #3770 We can cleanup from here. Apologies for back and forth, I should've coordinated better. :) |
No worries, I'll remove the change from this branch and rebase from main shortly. |
Here's a relevant issue we might want to follow: jsx-eslint/eslint-plugin-react#3140 |
Apologies for the noise, managed to accidentally close this while updating it. 😞 I've managed to repro the issue with a smaller sample and have a bit of time to dive into it now. |
@callingmedic911 That does look relevant, nice spotting! |
Based on jsx-eslint/eslint-plugin-react#3140 (comment), this is due to an incomplete implementation of an improvement to handle forwardRefs and other React features. I'm looking into what is required to resolve this. |
Checking back on this one. Any updates? |
No updates sorry. The upstream false-positive issue in eslint-plugin-react remains. I had a bit of a look at resolving it myself, but wasn't able to work it out. The changes in this PR will resolve the issue, but not in a nice way (results in duplicated type definitions in the code). The main alternative is to change back to using Omit (a better solution), but this would then require ignoring the linting issue until the false-positive is fixed. Happy to submit that as an alternative PR for comparison if you'd like. |
I've had another look at this. The issue in eslint-plugin-react is still present (as at release v7.28.0). However, given that react/prop-types is now disabled for the file, as in #4176, this can be merged to clear up the issue it was opened to fix ( As a note, I've copied @callingmedic911's FIXME note and added block disables of react/prop-types for the relevant types. This is safer than disabling the check for the whole file. It looks like a few unrelated violations of react/prop-types have managed to sneak in that will need cleaning up when the file disable is removed. |
Thanks for following up on this one @nzdjb! I just had a look at this workaround: jsx-eslint/eslint-plugin-react#3140 (comment), this looks good and it doesn't need many changes. How about we remove the FIXMEs and migrate all the forwardRefs' typing to something like this: -const TextAreaField = forwardRef<HTMLTextAreaElement, TextAreaFieldProps>(
+const TextAreaField = forwardRef(
(
{
name,
id,
// for useErrorStyles
errorClassName,
errorStyle,
className,
style,
// for useRegister
validation,
onBlur,
onChange,
...rest
- },
+ }: TextAreaFieldProps,
- ref
+ ref: ForwardedRef<HTMLTextAreaElement>
) => { With these changes, I think we'd be able to get this PR in and close #4135. What do you think? |
By the way, like you said, the changes I suggested are kind of unrelated. If you want I can do follow up PR. This PR is good to go. :) |
Sure thing. I've refactored all the components to the workaround you linked and re-enabled react/prop-types. The file passes linting now. |
type
property.type
property + Workaround for upstream react/prop-types lint rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks again @nzdjb for staying on top of this. :)
Based on feedback from @jtoar in #3725, take a different approach that means
type
is omitted from the hinting on TextAreaField.Not personally a big fan of this approach as it's not particularly DRY, but wasn't able to see a better way that didn't cause a failure on react/prop-types.
I have an inkling that this might be a bug in eslint-plugin-react, but wasn't able to track it down.