refactor(forms): Change typing to omit type property + Workaround for upstream react/prop-types lint rule

[nzdjb]

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.

[jtoar]

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 is already included in FieldProps, which (I think) makes it redundant:

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 type from hinting, the Omit<FieldProps<HTMLTextAreaElement>, 'type'> line surely does that I think. But again I'm not the best at TS so maybe there's something I'm missing—do you mind explaining your changes a bit more so I can follow your thinking?

[nzdjb]

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:

• Not omit type. This is state prior to this PR which you initially raised as an issue.
• Omit type and duplicate the addition of types. This is the state as per this PR.
• Set the linting issue to ignored on these types. I personally loathe doing this and would like to avoid it.
• Dig into eslint-plugin-react and potentially eslint itself to see if this is a bug. I think the detection probably is a bug as it doesn't make a lot of sense. I'm happy to do this, but currently have limited time, so also happy if anyone else wants to take a swing at it.
• Further digging into differing typing approaches to this rather complex type. I've had a few goes at this with no further progress. The type does seem to be correct, so this approach largely relies on this being some kind of parsing bug and a different expression of the type being parsed better.

[callingmedic911]

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 think Tobbe has already merged a fix. #3770 We can cleanup from here. Apologies for back and forth, I should've coordinated better. :)

[nzdjb]

No worries, I'll remove the change from this branch and rebase from main shortly.

[callingmedic911]

Here's a relevant issue we might want to follow: jsx-eslint/eslint-plugin-react#3140

[nzdjb]

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.

[nzdjb]

@callingmedic911 That does look relevant, nice spotting!

[nzdjb]

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.

[thedavidprice]

Checking back on this one. Any updates?

[nzdjb]

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.

[nzdjb]

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 (type property erroneously being present).

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.

[callingmedic911]

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 (type property erroneously being present).

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?

[callingmedic911]

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. :)

[nzdjb]

Thanks for following up on this one @nzdjb! I just had a look at this workaround: yannickcr/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:

With these changes, I think we'd be able to get this PR in and close #4135. What do you think?

Sure thing. I've refactored all the components to the workaround you linked and re-enabled react/prop-types. The file passes linting now.

[callingmedic911]

Looks good! Thanks again @nzdjb for staying on top of this. :)