fix: add ref prop type to withTheme

[zepatrik]

Reasons for making this change

typing for #1498 was missing as discussed in #1405
Is there any test that ensures the typing works?

Checklist

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• I'm adding or updating code
  • I've added and/or updated tests
  • I've updated docs if needed
• I'm adding a new feature
  • I've updated the playground with an example use of the feature

[epicfaace]

Thanks. I think for now, could you add a test in the material-ui package (because the tests there are in typescript) that uses forwarded refs with withTheme? Eventually, we'd want to move that test to rjsf/core once rjsf/core supports typescript tests.

[zepatrik]

I'm not sure I understand that correctly. The material-ui packages has "@rjsf/core": "^2.0.0" as a peer dependency so I probably can't add a test to use the local package, right? Or am I missing something there?
Alternative would be to add typescript as a devDependency in core and run tsc which would find any type errors. That way we could transfer the whole type test from DefinitlyTyped.

[epicfaace]

I was suggesting that we do something like this in the material-ui package:

import { withTheme } from "@rjsf/core";
// add a test using withTheme and refs here

Alternative would be to add typescript as a devDependency in core and run tsc which would find any type errors. That way we could transfer the whole type test from DefinitlyTyped.

This sounds like a better alternative, though, so let's do this instead!

[zepatrik]

sorry somehow fucked up the rebase, let me fix that.

[branflaker]

Running into the ref issues mentioned. It looks like it's been over a month since this was submitted. Just want to make sure it's not dead.

[zepatrik]

From my side this is finished, I just rebased my branch to the current master so you can use it.

[mattcosta7]

This typing isn't actually correct.

the actual return type here is actually more like

React.ForwardRefExoticComponent<React.PropsWithoutRef<FormProps> & React.RefAttributes<Form>>;

however, there's no way to handle this with the type system and the types of React.ForwardRefExoticComponent.

This will basically always make the

a <Form /> component, forwarding an any to the formData prop (always).

I suggested in #2279 (before seeing this pr) that returning typeof Form here might be a solution that aligns the resulting types more correctly with the function here?

[zepatrik]

Isn't FormProps generic? Why not pass down the generic just like I did? Your proposal

React.ForwardRefExoticComponent<React.PropsWithoutRef<FormProps> & React.RefAttributes<Form>>;

is just the same as my proposal, but without the generic?

[zepatrik]

Btw, I also added the types test https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-jsonschema-form/react-jsonschema-form-tests.tsx to see the effect of my changes. Maybe you can point to the test cases where you think the typing is wrong?

[mattcosta7]

@zepatrik will try to get a look at those tests in a little bit.

yes, FormProps is generic the issue is that

export function withTheme<T = any>(
        themeProps: ThemeProps<T>,
    ): React.ForwardRefExoticComponent<React.PropsWithoutRef<FormProps<T>> & React.RefAttributes<Form<T>>>;

this code uses T from the theme props as the value for the generic, when what's actually produced is a generic forward ref that takes some new type FormDataType (because I really don't like using T and U everywhere :-D) and a resulting <FormDataType>React.ForwardRefExoticComponent<React.PropsWithoutRef<FormProps<FormDataType>> & React.RefAttributes<Form<FormDataType>>> would be more correct, however to my knowledge, based on how ForwardRefExoticComponent is defined, it's impossible to return this from the withTheme function

The Form resulting from withTheme should accept a generic, not re-use the generic from withTheme.

returning typeof Form acts as though it's returning the generic class class Form<FormDataType> extend React.Component<FormDataType>, which maintains the ability to pass the formData generic into <WithThemedForm<FormDataType> />, which the current typings with ForwardRefExoticComponent cannot support

[mattcosta7]

// this test passes
export const genericFormDataExample = () => {
  type FormDataType = { field: string }
  const forwardedRef = React.useRef<Form<FormDataType>>(null);

  return <Form<FormDataType> schema={schema} formData={{field: ''}} ref={forwardedRef} />;
};

// this test fails to compile because Form as returned from withTheme is not a generic, but instead is a Form<any> always
export const withThemeExample = () => {
  type FormDataType = { field: string }
  const Form = withTheme({
    showErrorList: false,
    noValidate: false,
    noHtml5Validate: false,
  });
  const forwardedRef = React.useRef<Form<any>>(null);

  return <Form<FormDataType> schema={schema} formData={{field: ''}} ref={forwardedRef} />;
};

this tests, when modified like this errors, where as

[mattcosta7]

@zepatrik this was what I was getting at in the other comment about the return types for withTheme

[zepatrik]

So you want the return value of withTheme to be generic and not provide the generic when calling withTheme? That makes sense, but I guess as you said it is not really possible. My workaround does not fully fix this but I guess in most cases you will have the generic type of the final form anyways when you call the factory withTheme. Having this non-ideal typing is at least better than having none at all in my opinion.

[mattcosta7]

@zepatrik, no the generic to withTheme doesn't allow (and shouldn't) formData, so the formdata passed to the resulting form from withTheme is always any.

withTheme's T generic argument is just being inferred from the ThemeProps, which would in this example be

{
    showErrorList: false,
    noValidate: false,
    noHtml5Validate: false,
  }

which is correct

returning: typeof Form is the same as returning a Form class component (from typescripts knowlege), and does correctly allow my example above to work. It doesn't mirror the code here as directly, but it is properly to the spirit of this component factory.

it could also, similarly just return a <U>(props: FormProps<U> & React.RefAttributes<Form<U>>>) => JSX.Element, however this seems less clear than just returning a Form from withTheme (making it more correctly appear like a factory)

[zepatrik]

Ok I'm sorry, I am not really deep into this topic right now as the PR is already half a year old 😅
Do you want to take this over or fix it in a new one? I think it is a good idea to have the type test added as I did in this PR.
Feel free to open a new PR based on these commits.

[mattcosta7]

haha, no worries! I opened #2279 the other day, without seeing this first (I probably should have searched!). np, i'll pull some of these changes into that branch or those into this one and consolidate everything when I get a minute, thanks!

[epicfaace]

Great, thanks for consolidating those PRs @mattcosta7 . I'll go ahead and close this PR then, and we can add the changes in together with #2279.