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

Consider removing the autoField prop #980

Closed
radekmie opened this issue Jul 10, 2021 · 7 comments · Fixed by #1180
Closed

Consider removing the autoField prop #980

radekmie opened this issue Jul 10, 2021 · 7 comments · Fixed by #1180
Assignees
Labels
Area: Core Affects the uniforms package Type: Feature New features and feature requests
Milestone

Comments

@radekmie
Copy link
Contributor

Since we introduced AutoField.componentDetectorContext in #800 and released it in v3.1.0, the autoField prop of QuickForm (and all its descendant, including AutoForm) seems unreasonable. I fully agree with #977 (comment):

It's kind of strange for the implementation not to be passed down to the entire subtree (for me).

My point is, that this prop is there just to "initialize" the AutoField a given theme will be using. It's used only in one place and could be easily removed. However, when removed, we'll always render the "original" AutoField of a given theme, which only later will use the AutoField.componentDetectorContext to choose the component.

I believe there are cases where an entirely custom AutoField makes sense. But I cannot think of one that would not be possible without the autoField prop.

My idea looks as follows:

  1. Give this issue some time for people to comment on whether they'd have arguments for or against the removal of autoField.
  2. Deprecate autoField by showing a dev-only warning that it's going to be removed in the next major version.
  3. Remove it in the next major version.

I also thought about making the autoField prop automatically populate the AutoField.componentDetectorContext. The problem is that it won't work, we expect a function returning the component in the context, but a component in the autoField prop.

@macrozone
Copy link
Contributor

I was a bit surprised by this.

Wouldn't it be more logical that all Forms declare this provider with the default AutoField? And if you would pass autoField as prop, it would just pass a custom one to the provider?

That's in my opinion a far less surprising api than the new approach with forcing to wrap all forms with a context-provider.

@radekmie
Copy link
Contributor Author

radekmie commented Nov 3, 2021

The point of this change is to have exactly one API for doing so. We have to implement it somehow, so the context is here to stay. That means the autoField prop is redundant.

Wouldn't it be more logical that all Forms declare this provider with the default AutoField? And if you would pass autoField as prop, it would just pass a custom one to the provider?

That'd mean we always have to render the provider and that's not the case now. It probably wouldn't be as much of a problem, but it's always better not to render something if it's not needed.

That's in my opinion a far less surprising api than the new approach with forcing to wrap all forms with a context-provider.

You're not forced to use this provider in the first place - the library will work correctly without it. Also, you can do it once, at the root of your app - not for each form.

@radekmie
Copy link
Contributor Author

(We've had a team discussion about this issue, and here are the notes.)

We've decided to remove it in v4.

@mariusrak
Copy link
Contributor

I noticed deprecation in my code just now. So now I have to load e.g. Unstyled AutoField and let it load my custom AutoField? Is that right process? And is this the correct approach? Seems hacky to me and also seems like lading unnecessary stuff.

@kestarumper kestarumper self-assigned this Aug 15, 2023
@kestarumper
Copy link
Member

Hi @mariusrak,
There is no need to import AutoField from any theme.
Instead, we suggest you create your own custom AutoField with createAutoField(props => ...).

...and let it load my custom AutoField

If you already have your own AutoField, then you should be able to plug it into the createAutoField.

Hope that helps but feel free to ask more questions if needed.

@mariusrak
Copy link
Contributor

But how do I use this custom AutoField with AutoForm?

@kestarumper
Copy link
Member

But how do I use this custom AutoField with AutoForm?

@mariusrak You need to create your own extended AutoForm and override getAutoField method to return the AutoField created by createAutoField.

// @ts-expect-error Convoluted AutoForm types
class CustomAutoForm extends AutoForm {
getAutoField() {
return Field;
}
getNativeFormProps() {
return omit(super.getNativeFormProps(), 'autoField');
}
}
render(
// @ts-expect-error Convoluted AutoForm types
<CustomAutoForm
autoField={Field}
model={model}
onChange={onChange}
schema={schema}
/>,
);

This is because you need to assign that newly created AutoField to the AutoForm. This is the only place you need to do it. It's kind of as if you've created a new theme and started using it (the new AutoForm).

function Quick(parent: any) {
class _ extends QuickForm.Quick(parent) {
static Quick = Quick;
getAutoField() {
return AutoField;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core Affects the uniforms package Type: Feature New features and feature requests
Projects
Archived in project
5 participants