Skip to content
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

feat(templates): Allow customization per-field by using Registry string key #3881

Closed

Conversation

nagaozen
Copy link

Fixes #3695

Reasons for making this change

Currently, to per-field customize the template, it is necessary to send the function/class of the component in the uiSchema, breaking the JSON nature of the uiSchema. It's not possible to provide just the registration key.

This PR fixes this issue by allowing the consumer of the library (the one with access to changing only schema, uiSchema and formData) to choose a different template provided to the form by the developer.

@ahkhanjani, could you please review?

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@nagaozen nagaozen mentioned this pull request Sep 26, 2023
1 task
@nagaozen
Copy link
Author

nagaozen commented Sep 27, 2023

Anyone to help the review this PR? @heath-freenome?

@heath-freenome
Copy link
Member

heath-freenome commented Oct 6, 2023

@nagaozen This fix makes sense in the basic form, only it doesn't really help the situation where someone wants to build multiple templates to override. Because, currently, the TemplateTypes interface only supports the known template names, by simply adding a new (say) ObjectFieldTemplate to the template prop in the Form ALL ObjectFieldTemplates will now use this implementation. So basically, by simply adding the updated implemention as the ObjectFieldTemplate in templates it will replace the OOTB implementation provided by core or the theme you are using. So picking the name inside of a uiSchema is effectively the same as not providing it at all.

If what you are hoping to support is something like how fields and widgets work (i.e. allowing a user to build several ObjectFieldTemplate implementations and override the uiSchema to select from one of the implementations), then more work is necessary.

For one, you will have to extend the TemplateTypes interface to support custom named templates for a specific type (note fields and widgets all have the same Typescript type where as templates have unique Typescript types). This is no easy task and requires an enhancement to the design of the templates scheme in RJSF. Come to the next weekly meeting if you'd like to talk about it.

@nagaozen
Copy link
Author

nagaozen commented Oct 9, 2023

@nagaozen This fix makes sense in the basic form, only it doesn't really help the situation where someone wants to build multiple templates to override. Because, currently, the TemplateTypes interface only supports the known template names, by simply adding a new (say) ObjectFieldTemplate to the template prop in the Form ALL ObjectFieldTemplates will now use this implementation. So basically, by simply adding the updated implemention as the ObjectFieldTemplate in templates it will replace the OOTB implementation provided by core or the theme you are using. So picking the name inside of a uiSchema is effectively the same as not providing it at all.

If what you are hoping to support is something like how fields and widgets work (i.e. allowing a user to build several ObjectFieldTemplate implementations and override the uiSchema to select from one of the implementations), then more work is necessary.

For one, you will have to extend the TemplateTypes interface to support custom named templates for a specific type (note fields and widgets all have the same Typescript type where as templates have unique Typescript types). This is no easy task and requires an enhancement to the design of the templates scheme in RJSF. Come to the next weekly meeting if you'd like to talk about it.

@heath-freenome, thank you for your response. My idea with this patch is to use the existing mechanism for customizing templates per field as described in https://github.com/rjsf-team/react-jsonschema-form/blob/main/packages/docs/docs/advanced-customization/custom-templates.md. The difference is that, instead of receiving the function directly in the uiSchema body, the developer can choose to insert another key in the template registry that can be used instead of manually sending the entire component manually (think about using the same customized template in different fields). I'm sure the patch works as we are already running it on production. For example, see how we are using the layout template at https://actions.looplex.com/code/custom-widgets-showcase for both Shipping Address object and Strings. There's also an example of our autofill field working: try to type "01302001" on the CEP field and TAB for the next field :-)

image

I'll be glad to expose the reasoning behind the patch in the next weekly. Thank you for the invitation.

@@ -86,4 +86,19 @@ describe('getTemplate', () => {
expect(getTemplate<typeof name>(name, registry, uiOptions)).toBe(CustomTemplate);
});
});
it('returns the template from registry using uiOptions key when available', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add another set of tests for custom names

typeof uiOptions[name] === 'string' &&
Object.hasOwn(templates, uiOptions[name] as string)
) {
const key = uiOptions[name];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To truly support things in a way that matches how widgets and fields updating the TemplateTypes type in @rjsf/utils/src/types.ts to add this one additional line is all the extra work you will need to do:

  /** Allow this to support any named `ComponentType` or an object of named `ComponentType`s */
  [name: string]: ComponentType<any> | { [name: string]: ComponentType<any> };

Copy link
Member

@heath-freenome heath-freenome Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I was wrong. It will require creating a new interface called RJSFBaseProps with the definition:

export interface RJSFBaseProps<T = any, S extends StrictRJSFSchema = RJSFSchema, F extends FormContextType = any> {
  /** The schema object for the field being described */
  schema: S;
  /** The uiSchema object for this description field */
  uiSchema?: UiSchema<T, S, F>;
  /** The `registry` object */
  registry: Registry<T, S, F>;
}

Then you will want to update all of the interfaces and types that the TemplateTypes inner props that have ComponentType<XXX> to make the XXX interface/type extend RJSFBaseProps

Then add the following to the top of the TemplateTypes interface instead of what I mentioned above:

/** Allow this to support any named `ComponentType` or an object of named `ComponentType`s */
  [name: string]: ComponentType<RJSFBaseProps<T, S, F> | { [name: string]: ComponentType<RJSFBaseProps<T, S, F> };

@@ -76,7 +76,7 @@ render(
);
```

You also can provide your own field template to a uiSchema by specifying a `ui:ArrayFieldTemplate` property.
You also can provide your own field template to a uiSchema by specifying a `ui:ArrayFieldTemplate` property with your Component or a string value from the `Registry`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding at least one example of how to use an alternate template with a different name in the uiSchema here and possibly all the sections you've updated below

@heath-freenome
Copy link
Member

@nagaozen This fix makes sense in the basic form, only it doesn't really help the situation where someone wants to build multiple templates to override. Because, currently, the TemplateTypes interface only supports the known template names, by simply adding a new (say) ObjectFieldTemplate to the template prop in the Form ALL ObjectFieldTemplates will now use this implementation. So basically, by simply adding the updated implemention as the ObjectFieldTemplate in templates it will replace the OOTB implementation provided by core or the theme you are using. So picking the name inside of a uiSchema is effectively the same as not providing it at all.
If what you are hoping to support is something like how fields and widgets work (i.e. allowing a user to build several ObjectFieldTemplate implementations and override the uiSchema to select from one of the implementations), then more work is necessary.
For one, you will have to extend the TemplateTypes interface to support custom named templates for a specific type (note fields and widgets all have the same Typescript type where as templates have unique Typescript types). This is no easy task and requires an enhancement to the design of the templates scheme in RJSF. Come to the next weekly meeting if you'd like to talk about it.

@heath-freenome, thank you for your response. My idea with this patch is to use the existing mechanism for customizing templates per field as described in https://github.com/rjsf-team/react-jsonschema-form/blob/main/packages/docs/docs/advanced-customization/custom-templates.md. The difference is that, instead of receiving the function directly in the uiSchema body, the developer can choose to insert another key in the template registry that can be used instead of manually sending the entire component manually (think about using the same customized template in different fields). I'm sure the patch works as we are already running it on production. For example, see how we are using the layout template at https://actions.looplex.com/code/custom-widgets-showcase for both Shipping Address object and Strings. There's also an example of our autofill field working: try to type "01302001" on the CEP field and TAB for the next field :-)

image

I'll be glad to expose the reasoning behind the patch in the next weekly. Thank you for the invitation.

After thinking more about it, I believe that changing the TemplateType and the associated types like I've noted above will be enough

@heath-freenome
Copy link
Member

@nagaozen Any progress on this?

@nagaozen
Copy link
Author

@nagaozen Any progress on this?

Hi @heath-freenome ! I am deep diving in a vital feature in my business, but asked if someone in my team could implement the static typing requirements for us. If nobody cares to conclude the ticket I'll come to finish it asap. Thanks for the insights

@nickgros
Copy link
Contributor

@nagaozen there's a similar request in #3962 with a slightly different API than you've proposed. What do you think of this?

@nagaozen
Copy link
Author

@nagaozen there's a similar request in #3962 with a slightly different API than you've proposed. What do you think of this?

Hi @nickgros,

Ultimately, the goal is the same: "Customize the ObjectFieldTemplate through the uiSchema." The use case presented by @elevesque-nexapp is similar to what we are facing here: "The backend is produced by an engineering team and the frontend developer provides only the schema, uiSchema, and formData." This segregation of concern is what makes this feature necessary. Because backend developers can document these alternatives so that frontend engineers can use them. As for the suggested approach (nesting the options in objects), it differs from how the library already handles custom widgets and fields. Therefore I believe this current approach superior in terms of DX.

@elevesque-nexapp
Copy link

I agree the solution proposed here is better aligned with how customization works for other fields. I'd be perfectly happy to see this solution merged!

@heath-freenome
Copy link
Member

@nagaozen Seems like your solution is the winner. Let's get those typing changes made and then we complete your PR

@smithaitufe
Copy link

@nagaozen Do you think you will have time for this?

Maybe I can play with this to see if I can get it to a mergeable state

@nagaozen
Copy link
Author

@nagaozen Do you think you will have time for this?

Maybe I can play with this to see if I can get it to a mergeable state

Oh, please be my guest. Its on my todo list for a considerable time. I believe this will greatly improve the library.

@lucasmcht
Copy link

Hello, any news about this PR ?

@lucasmcht
Copy link

@nagaozen Do you think you will have time for this?
Maybe I can play with this to see if I can get it to a mergeable state

Oh, please be my guest. Its on my todo list for a considerable time. I believe this will greatly improve the library.

I would gladly make the required changes but I don't have the rights to update your branch

@heath-freenome
Copy link
Member

@Oddwerth You could create your own fork and then cherry pick these changes from theirs

@lucasmcht lucasmcht mentioned this pull request Oct 28, 2024
8 tasks
@lucasmcht
Copy link

@heath-freenome I have created a new PR with the changes here : #4352

@heath-freenome
Copy link
Member

Fixed by #4352

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

current per-field customization break JSON rules
6 participants