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

Incorrect field.state.value type #705

Closed
RobertVillalba opened this issue May 14, 2024 · 5 comments
Closed

Incorrect field.state.value type #705

RobertVillalba opened this issue May 14, 2024 · 5 comments
Labels

Comments

@RobertVillalba
Copy link

RobertVillalba commented May 14, 2024

Describe the bug

I'm wondering if this was a design decision (I've seen it in RHF), but I feel like it is still incorrectly typed somewhere. A Field's field.state.value will have whatever type the useForm hook is given for that field. This is often correct, but when a default value is not provided the field can actually be undefined. This can be the cause behind components switching between controlled/uncontrolled or just general issues from having invalid types.

Your minimal, reproducible example

https://codesandbox.io/p/sandbox/tanstack-form-d2rpvg?file=%2Fsrc%2FApp.tsx%3A20%2C7

Steps to reproduce

Hover over field.state.value

Expected behavior

As a user, I expected TS values to be correct, but I'm seeing that this field has a value that is not correctly captured by the provided TS constraints.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • OS: windows
  • Browser: chrome

TanStack Form adapter

react-form

TanStack Form version

v0.19.5

TypeScript version

v5

Additional context

No response

@crutchcorn
Copy link
Member

This is typed properly and this is not a bug.

What you're doing by type-casting useForm is that you're explicitly telling us what the shape of form.state.value is. If we were to change this by adding conditionality, we'd be breaking your own guidance on the type should be.

Instead, I'd propose passing Partial<YourType> here.

@RobertVillalba
Copy link
Author

I'm not sure I fully understand, could you explain a bit more? Is the argument here that there is no way to tell useForm what the type of form.state.value (value) should be? I'm not sure if you're stating a difference between "type" and "shape" in your comment above. It seems like I should be able to specify that this form has a type of { s: string } vs { s?: string } and that the first variant here should force me to pass it in a default value so that the value matches what I tell it I'd like the form to be. I've seen a lot of instances of RHF where users think that they are defining the type of the value only to later find a bug in their code because the type system did not tell them that a given field would ever be undefined after they accidentally passed in a default value that did not match their generic type.

Also did not fully follow what you meant by breaking my own guidance? And the suggestion of passing Partial<...> does not help me with the scenario where I'd like to explicitly state the type of my form's data.

@crutchcorn
Copy link
Member

Could you explain a bit more?

Sure! Lemme start with an analogy, since I think it's important to communicate the headspace you need to get into for TanStack projects to really click (IMO, anyway):


Say I'm at a party and a person and I are meeting for the first time. I assume, based on what their friends are calling them at this party, that their name is "Benny". But suppose that this is a semi-formal event and they prefer to be called their full name "Benjamin" in those contexts.

This is the difference between type inferencing and explicit type passing:

Inferencing might be accurate in some/most instances based on context clues, but when provided with explicit instructions, we must throw away the existing inferenced data

This isn't just a courtesy in the programming world, either, TypeScript literally does not provide us the inferred data once you pass explicit an type to a generic.

function identity<T>(v: T) {
    return v;
}

// Inferred as `1`, not explicit
identity(1);
// Explicitly `number`, `1` inferred type is lost
identity<number>(1);

Moreover, even if we could, consider what would happen if I decided to shorten Benjamin's name to "Ben" without asking. That might come across as rude or jarring, as I was explicitly told by Benjamin what they wanted to be called.

This, too, is similar to our type system for Form. If you explicitly tell us "s is a string" and we type cast it to be "s is a string OR undefined" without asking your permission (ala, some kind of "partial" flag or something) then that behavior will cause headaches and problems in a different way than you're running into.


Hopefully this helps explain a bit of why it works the way it does in TanStack Form? :)

@RobertVillalba
Copy link
Author

Sure I get that providing an explicit type will tell TS to use that type, but I think that only fairly applies in the example because the explicit type is wider than the passed in literal. For example TS would not allow us to say

identity<1>(Math.random());

And I think this is essentially one of the two possible solutions that could be considered here. If the user tells the library that this will be the data type, but it does not provide the correct data to satisfy that type then they should be warned about this. Personally this would be my preference, but perhaps there is a use case for a user providing a type and still not passing in that type. I see that your library will correctly prevent me from passing a default of {} in my original example which I think is great. It seems to me that the final behavior stems more from the fact that this field is optional so perhaps a closer toy example would be

function identity2<T>(v?: T) {
    return v;
}

And indeed TS would now type this return type as T | undefined which I still think is correct. At no point does the function here give me a type that says number but can actually be undefined. I would consider this the second solution to the issue. I think if we allow undefined instead of T then the user should see that the value of a field is either the typed value or possibly undefined. Unless I am also misunderstanding and users of this library should never explicitly pass a type to useForm? I think that would also be a fair argument, but I don't think that's the actual case. In the most "flexible" and correct approach I'd say something like

function identity2<T>(): T | undefined;
function identity2<T>(v: T): T;
function identity2<T>(v?: T) {
    return v;
}

type D = { s: string }
const d: D = { s: "hello" }

// Inferred as D
identity2(d)
// Inferred as unknown
identity2()
// Infered as D | undefined
identity2<D>()
// Infered as D
identity2<D>(d)

would provide a correct solution, but I'm not sure all of this is needed if there isn't a use case for the above scenario.

Again usually when we tell a thing that its type will be T as a generic this is not the same thing as using the keyword as and just telling the TS system to "trust us". If the type was being widened as in the example you showed, it would still be safe and I would be in agreement, but in actuality the user is being an told incorrect statement about the value of a field. And this is not an isolated issue. I've seen several bugs arise from similar loose types with RHF. This library looks great and I'm really looking forward to something that can finally replace RHF and has actually tight and correct types. I don't mean any disrespect by that, just honestly see a lot of potential in this and would like to see all of the shortcomings and things I've been burned with elsewhere resolved here.

@crutchcorn
Copy link
Member

Ahh okay, most of the time I get requests akin to this, it's often because someone is asking for incorrect type narrowing <{s: string}>({s: undefined} as never) but you're specific edgecase is with an empty function call if I'm understanding correctly.

The problem with the () problem here is that we can't easily infer the empty usage of () and pass it all the way through to field.state.value. Just the complexity of our types.

I'm gonna keep this closed as this feels pseudo intentional to me, but if you're able to make a PR that fixes empty functions specifically we'll likely merge it

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

No branches or pull requests

2 participants