-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fields are always serialized as strings, messages not accepted by the server #3
Comments
Thanks for raising the issue! That is actually a great point that I totally missed. The extension should be able to serialize values correctly without the need for server-side workaround. I'll see what can be done Unfortunately, fixing this might be a breaking change, so I probably should also put this on NPM so people won't have to deal with linking to my master branch. |
I've given this problem some thoughts, and here are my ideas: Automagically detect value type. That is, if a string can be converted to a number, just convert it. Very simple, but can be unpredictable and undesirable in cases, when we do want our value as a string (see phone number and various government identifiers). So, this option is basically a no-go Detect value type based on input type. There we have a bit more control, but basically the same problem, just because input is validated as a number does not mean that we want to serialize it as one. There is also an opposite problem. Sometimes numbers can be entered in non-number inputs (hidden input, text input with custom validation). A sophisticated detector, based on inputmode/pattern attributes would be better, but still not perfect, also would not cover JS-driven validation/masking. Also, it would require custom form-extraction code to maintain. Overall, slightly better then first option, but still does not solve the problem in its entirety (or at least it appears to me this way). With that in mind, adding custom, extension specific attributes to control value type seems like the best combination of control and predictability. It also would not, in fact, be a breaking change, which is great news <!-- throws validation error if value can not be converted to a number, the page code must have validation prior to submit -->
<input type="text" signalr-type="number" inputmode="numeric" pattern="[0-9]*"> I'd love to get some input on this, especially on option two. I might be overestimating complexity of input type detection and it could actually be the best option |
My gut is that you could cover many of the common cases using option two, but that it likely won't cover all of them and then you'd need another option. I'm not yet super deep into htmx or signalr so some of these things may not make sense, but I have a few thoughts:
|
This is great, thank you so much! |
You asked for feedback on this, so here I am (a week late - oops!). :) My experience with Signalr is relatively thin, so I really don't have a whole lot to add. I would shy away from automatic detection unless there's some prevailing industry standard around it; someone saying their name is 53453 should come across as "53453". I like the custom attribute idea; however, OP's point 3 is also well-made. |
So sorry for radio silence. I contemplated on this issue for a bit and decided that you folks are right. Server-side conversion makes the most sense, as dealing with all possible edge-cases with conversion on the client is going to be a wild ride. Lord, so many edge-cases... I'll update the doc to highlight this requirement |
I suspect this is likely something to be addressed via an update to the readme rather than a feature request. I have a server side method that takes an int parameter. I'm using signalr-send on a form element and getting the value for that parameter from a hidden input. Until I enabled debug signalr logging I didn't understand why the breakpoint in my server code was never being hit (and the server method never being executed). It looks like all params get sent over as strings, so the server side stuff needs to only take strings, unless I've overlooked something.
Changing my params in the server code to strings and just parsing the int out on that side works just fine, but it wasn't obvious at first what was going on.
The text was updated successfully, but these errors were encountered: