refactor: types for withTheme

[mattcosta7]

Reasons for making this change

fixes #2135

The different themes were defining their main form (generated from the withTheme function from core) with type annotations that erased the 'ref' property. By treating withTheme as an enhancer that returns a Form, the Forms from the themes maintain their generic typings, as well as the ref properties they should have.

I tested a few alternatives, and found this to be the most ergonomic in usage with the best typings to least error ratio.

• Returning a generic component function seemed wrong, and 'erases' that this ref is passed through to a class.
• Returning a ForwardRefExoticComponent or the like removes the generic nature of the types Form already uses in core, and this is something super powerful for ensuring the data and expected data across various methods stays in sync (I use this in projects relying on rjsf/core and material, so having that back in material would be great).

I added simple tests for this in the separate typescript projects here as a canary for regressions. If the typings break, these should break as well. I didn't think it worth trying to test this in core, but I can look at that if desirable.

Additionally, I had prettier on without noticing in one of the files, i'm happy to undo those changes, but only noticed them after pushing, so i'll leave for now - likewise I can also run prettier on the other files if desired in a followup.

If this is related to existing tickets, include links to them as well.

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

[mattcosta7]

Given that this makes the typings for the exports to material, bootstrap and fluent more sound, it's possible they'd expose issues in current code for users. I don't believe this should cause any churn, but it's possible it could

[epicfaace]

Thanks for the PR!

• I'm not too familiar with ForwardRefExoticComponent, but it looks like forwardRef does usually return a ForwardRefExoticComponent. Would it be misrepresenting the nature of forwardRef / could it cause any issues if we return a typeof Form instead?
• What kinds of backwards compatibility issues do you think users might face? If there are issues, we should probably add this as a breaking change.

[epicfaace]

Why is this import needed?

[epicfaace]

Checking on this @mattcosta7 ?

[mattcosta7]

it's not. i'll drop that update. I actually don't think most of these imports are needed

[epicfaace]

How exactly does this snapshot ensure that the ref property is properly passed down? What exactly are we testing here -- just to make sure that the rendering doesn't crash? (if so, we wouldn't need a snapshot)

Also, I'm having trouble seeing why the old typings would break this test. What would happen with the old types?

[mattcosta7]

@epicfaace the reason for not using Forward RefExoticComponent is that the typings for it (in @types/react) prevent a generic

In core the class Form allows end users to properly type things based on the shape of form data.

The react typings for ForwardRefExoticComponent will swallow that generic. I think that maintaining it is more important than returning that type as is (this way users can pass the generic to the component as necessary), and provide more accurate intellisense

With theme is really just an factory returning a Form component, with some defaults. I think this typing reflects that intent, albeit it hides the implementation a little

[mattcosta7]

As far as comparability -

In core components generated from withTheme would have only allowed a Formdata type of any, so the internal handlers referencing those types would have always been any (as would refs related).

It's possible that a user could have passed a formData value that gets inferred elsewhere - in a submit or change handler which might reference properties that don't exist in formData, and therefore cause a compilation error. Since the old typing would have been any for these, by default, that would not have occured.

For the material/fluent/etc themes, no generic was passed and so it's unlikely any issue could have existed there.

I agree these tests aren't necessarily important, but they ensure that theres a ref passable (otherwise no value could be set as a ref) and that the component accepts a generic. Both of these would error without that possibility - although I would like to find a good way to test this in core as well (although I'm not familiar yet with how/if we can easily test tax with the mocha setup there)

[epicfaace]

I agree these tests aren't necessarily important, but they ensure that theres a ref passable (otherwise no value could be set as a ref) and that the component accepts a generic. Both of these would error without that possibility - although I would like to find a good way to test this in core as well (although I'm not familiar yet with how/if we can easily test tax with the mocha setup there)

Ok, great -- in that case, would you be fine removing just the snapshots (so there's less snapshots for us to maintain)? Or do you think it's important to keep the snapshots themselves for the canary functionality?

[mattcosta7]

Not at all, I'll drop those to just make sure something passes, and add a bit more description to those tests so it's clearer what they're being used for

…

On Fri, Mar 19, 2021, 21:27 Ashwin Ramaswami ***@***.***> wrote: I agree these tests aren't necessarily important, but they ensure that theres a ref passable (otherwise no value could be set as a ref) and that the component accepts a generic. Both of these would error without that possibility - although I would like to find a good way to test this in core as well (although I'm not familiar yet with how/if we can easily test tax with the mocha setup there) Ok, great -- in that case, would you be fine removing just the snapshots (so there's less snapshots for us to maintain)? Or do you think it's important to keep the snapshots themselves for the canary functionality? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2279 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBXYATW6MFRZFLS5O5NUPDTEP2XNANCNFSM4ZJ4LLEA> .

[mattcosta7]

@epicfaace updated tests here.

[mattcosta7]

Agreed, it would be, however the following lines reference a property on that object, which typescript won't infer from the expect().toBeTruthy statement We could use non null assertions instead of wrapping in the if statement Ala Expect(ref.current!.submit).toBeDefined()

…

On Sat, Mar 20, 2021, 16:59 Ashwin Ramaswami ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/material-ui/test/Form.test.tsx <#2279 (comment)> : > + formData={{ + field: "", + }} + schema={{ + type: "object", + properties: { + field: { + type: "string", + }, + }, + }} + /> + ); + + expect(ref.current).toBeDefined(); + if (ref.current) { I think it would be simpler just to do, instead of the else statement, expect(ref.current).toBeTruthy() // Expect ref to be properly forwarded — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2279 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBXYAWOU3RT7XVFPZEARDTTEUEDJANCNFSM4ZJ4LLEA> .

[mattcosta7]

I pulled in the type tests from @zepatrik 's branch #1915

[epicfaace]

@mattcosta7 , since this is technically a breaking change, can you add an entry to the 3.x upgrade guide that explains to users what has changed and what they may need to change in their own code?

The guide isn't yet on the master branch (it's in #2211), but here's the version of the guide that's in that branch: https://github.com/rjsf-team/react-jsonschema-form/blob/26d34051bbc063777645aa825ed4192e260930ab/docs/3.x%20upgrade%20guide.md You can just create the docs/3.x upgrade guide.md file on this branch.

[medfreeman]

edit: add back children property

Here's my take on this, in use in a company project for a year.
It solves the generics problem too.

For reference, it is then used in a module that has form factory filling specific needs in our project (additional default behavior, external styling additions, data type references in event handlers, error message translation, etc..).

withTheme.tsx

import React, { ReactElement, ReactNode, Ref, RefAttributes, forwardRef } from 'react';

import JsonSchemaForm, { FormProps, ThemeProps } from '@rjsf/core';

type TFormWithTheme = <T = {}>(
  p: FormProps<T> & { readonly children?: ReactNode } & RefAttributes<JsonSchemaForm<T>>
) => ReactElement | null;

export function withTheme<T = {}>(themeProps: ThemeProps<T>): TFormWithTheme {
  return forwardRef(
    (props: FormProps<T> & { readonly children?: ReactNode }, ref: Ref<JsonSchemaForm<T>>) => {
      const { fields, widgets, children, ...directProps } = props;

      return (
        <JsonSchemaForm<T>
          {...themeProps}
          {...directProps}
          fields={{ ...themeProps.fields, ...fields }}
          widgets={{ ...themeProps.widgets, ...widgets }}
          ref={ref}
        >
          {children}
        </JsonSchemaForm>
      );
    }
  ) as TFormWithTheme;
}

usage.tsx

const FormWithMUITheme = withTheme(MuiTheme);

type TForm = <T = {}>(
  p: FormProps<T> & RefAttributes<JsonSchemaForm<T>>
) => ReactElement | null;

export const Form: TForm = forwardRef(
    <T extends unknown = {}>(props: FormProps<T>, ref: Ref<JsonSchemaForm<T>>) => {
      const { whatever, ...otherProps } = props;
    
      /* various operations */

      return (
        <FormWithMUITheme<T>
          ref={ref}
          {...otherProps}
        />
      )
    };
) as TForm;

[epicfaace]

How is a type "given"? Should we show an example code of how to do so?

[jorisw]

Ping @mattcosta7

Would be nice if we can merge this PR.