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

Explicit form types introduced on v3.6.0 #1083

Open
afmfe-iul opened this issue Jan 10, 2022 · 4 comments
Open

Explicit form types introduced on v3.6.0 #1083

afmfe-iul opened this issue Jan 10, 2022 · 4 comments
Labels
Area: Core Affects the uniforms package Type: Feature New features and feature requests
Milestone

Comments

@afmfe-iul
Copy link

Hey there!
On our project we use uniforms and TypeScript (v4.5.4) extensively and last week we upgraded uniforms / uniforms-semantic / uniforms-bridge-json-schema from v3.5.5 to version v3.7.0, which caused a couple of type errors. After going through the changelog we found out that very likely #1003 (from v3.6.0) is where the issues started.

Specifically we are getting errors on AutoForm prop types:

  1. className is not a valid prop, however adding one works;
  2. onChangeModel is not a valid prop;
  3. onSubmit is of type (model: DeepPartial<Model>) => void | Promise<any>, however we always assumed that it would be (model: Model) => void | Promise<any> because if you passed validation (and you typed everything correctly) by the time you reach submission your model would be matching the required type.

Here is a simple example on CodeSandbox (however sometimes the compiler errors don't show because their compiler is fixed at 4.4.4 it seems). If you try this example outside CodeSandbox, with TS 4.5.4, you might get the same results as we.

Currently the workaround we are using is the one sugest on the FAQ (use of any).

Would love to get your thoughts on the points above!

@radekmie radekmie self-assigned this Jan 14, 2022
@radekmie radekmie added the Type: Feature New features and feature requests label Jan 14, 2022
@radekmie radekmie added this to the v3.x milestone Jan 14, 2022
@radekmie
Copy link
Contributor

Hi @afmfe-iul! You've found the correct suspect here - #1003. And yes, as a current workaround, casting the form to any.

Now, let's get to the errors step by step:

  1. className should be inferred from the HTMLFormElement props, but it's not now. I see that as a proper improvement that we could do. However, it's possible to override the actually rendered component, so we should somehow carry this information.
    • Another generic argument makes sense, but it's hard to override.
  2. onChangeModel is added by the AutoForm and this problem probably hides the correct type here.
  3. I'm still unsure on how to think about that. On the one hand, we are aware that we start with an empty object - not a valid model. But on the other, we should trust the validator in this manner.

I'd love to hear your suggestions here: @afmfe-iul, @wadamek65, @kestarumper, @Monteth.

@wadamek65
Copy link
Contributor

Ad. 3: I'm also not sure, because even though a passed validation should ensure the model to be present there in most cases, there are situations where that is not true, for example in custom bridges. Maybe another approach would be to not try to type this at all and instead force the user to? Like make it an unknown type and make the AutoForm accept a generic type of the expected model? Or keep it partial and implement the latter if possible?

@radekmie
Copy link
Contributor

Small update: we'll keep it the way it is for a little more time and discuss it internally once again. Especially as we're already discussing v4.

If you have any ideas or points about this topic - do let us know here.

@afmfe-iul
Copy link
Author

Hey @radekmie and @wadamek65 ! Thanks for the explanation on each point and for the update.

Unfortunately I don't have suggestions on how to solve these issues (my knowledge of your source code is not deep), however generics is something we use frequently on our project and are used to deal with with other packages, it would be an awesome first step away from the current fix.

For us types are essential, they greatly increase our confidence and speed, so we try to have as little as possible occurrences of any on our project, we would value a lot if uniforms would move in a direction that supports super accurate types!

PS- For context, currently we have 22 forms, some with 20-30 fields and conditions, all using JSONSchemaBridge from "uniforms-bridge-json-schema", without types or with hacky solutions we feel there's a lot of room for error!

@radekmie radekmie added the Area: Core Affects the uniforms package label Jun 10, 2022
@radekmie radekmie modified the milestones: v3.x, v4.0 Oct 7, 2022
@radekmie radekmie removed their assignment Nov 4, 2022
@radekmie radekmie moved this to To do in Open Source Nov 18, 2022
@kestarumper kestarumper modified the milestones: v4.0, v4.x Apr 12, 2024
@piotrpospiech piotrpospiech moved this from To do to On Hold in Open Source Apr 12, 2024
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
Status: On Hold
Development

No branches or pull requests

4 participants