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

fix(getElementInput): Select type number for HTMLSelectElement #259

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

markojerkic
Copy link

Problem

When field type is number and Field component is bound to an HTMLSelectElement, form will always receive undefined, as HTMLSelectElement does not have a property valueAsNumber.

Proposed solution

Add a check if element is HTMLSelectElement and input type is number. If true, then return parseFloat(value).

What I did

I started by implementing the Solid solution, updated the examples for Solid and then pretty much just copy-pasted the fix for React, Preact and Qwik. I tested them all, they seem to work just fine.

@fabian-hiller
Copy link
Owner

Thank you! The bigger question here is if we should support number for <select /> as the library tries to be close to the HTML spec. How about transform it to a number manually afterwards?

@fabian-hiller fabian-hiller self-assigned this Oct 23, 2024
@fabian-hiller fabian-hiller added the question Further information is requested label Oct 23, 2024
@markojerkic
Copy link
Author

Sure, that could be an option. But in the current state, it is very easy to assume that <select /> with v.number() would work, as there is no type error. It is a bit difficult to catch the bug, as it always just returns undefined.
I did catch the bug on my side and tried the solution you're proposing (v.string(), and v.transform()).
I guess that I can see what you're saying. You would have to probably add the same logic for the boolean as well.

One solution is perhaps to create a Gotchas section in the documentation showing all these behaviours. As currently, only the HTMLInputElement supports all data types (date, number, string, boolean and file).

Another solution would be to create safeguards. For example, if element instanceof HTMLSelectElement && type !== "string", throw Error("HTMLSelectElement supports only string data types. Consider adding a transform pipeline to your schema.")

@fabian-hiller
Copy link
Owner

Thanks for the detailed answer! I still try to maintain Modular Forms, but I hardly add new features and extend the documentation because I have almost no time besides my studies and Valibot. But I still plan to rewrite Modular Forms one day. If this issue is a blocker for you, I will consider merging it. Otherwise I will consider your feedback when rewriting the library.

@fabian-hiller fabian-hiller added enhancement New feature or request and removed question Further information is requested labels Oct 24, 2024
@markojerkic
Copy link
Author

It is not a blocker. Thank you for having a look at this PR. Transforming is a valid solution, although it wouldn't hurt to be a bit more explicit about what you can and cannot do.

Feel free to close this PR if you don't feel it brings value.

@fabian-hiller
Copy link
Owner

Thank you! I think it brings value. I will leave it open for now. I am just careful with my time as other projects like Valibot are a bit more important at the moment.

@fabian-hiller fabian-hiller force-pushed the main branch 6 times, most recently from d40777c to 750b759 Compare January 3, 2025 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants