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

Avoid type widening with enum-typed values in FormValue #541

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmcw
Copy link

@tmcw tmcw commented Mar 25, 2024

The current generic type for FormValue is this:

export type FormValue<Schema> = Schema extends
	| string
	| number
	| boolean
	| Date
	| bigint
	| null
	| undefined
	? string | undefined

In this, if you have an enum or constant string type, then it will get widened into a string type. Turning Date and null types into string | undefined makes sense, but it is kind of just inconvenient to lose type information if you have an enum.

This tweaks the generic type so that if you have a type that extends string, then the FormValue generic uses that type instead of widening to string.

@edmundhung
Copy link
Owner

edmundhung commented Apr 1, 2024

This is tricky... because conform does not validate the form value. The type you got is more or less a faith that the type should be mostly correct if you are using Conform properly. But it couldn't stop you from putting a hidden input with an invalid enum which will be reflected on form value regardless. 😅

Maybe it is alright to made the same assumption with enum here... What do you think?

@jansedlon
Copy link
Contributor

jansedlon commented Apr 2, 2024

I think that that's what typescript's about, constraining things to be something we want even though Javascript allows to do about anything. If you use typescript incorrectly or if you make a mistake and mistakenly widen a type to things you don't want to, you will fuck yourself up anyway :D

On the other hand since inputs are strings / files by design.. I would slightly incline to have it widened and put the responsibility on the developer. I know, I also don't like not having the convenience but.. It forces the developer to put more checks in place.

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.

None yet

4 participants