Renerick / htmx-signalr

htmx extension for interacting with ASP.NET Core SignalR connections directly from HTML
MIT License
39 stars 5 forks source link

Fields are always serialized as strings, messages not accepted by the server #3

Closed rpierry closed 1 year ago

rpierry commented 1 year ago

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.

Renerick commented 1 year ago

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.

Renerick commented 1 year ago

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

rpierry commented 1 year ago

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:

  1. can you do nothing and then people who need the params set as different types could just hook beforesend and alter the values of the filtered parameters? I'm not sure if that would even work, but it seems like it might by just looking through the code.
  2. I'm not sure if your proposed signalr-type gives you much more flexibility than your option 2. Things that are primitive types potentially just map to inputmodes or input types so the new attribute could be duplicative. I will think about this one more
  3. putting my purist hat on, I'm almost inclined to argue that treating things as strings is the right behavior. I say this because just like client validation is an optional thing and not to be trusted, data type conversions (or convertability) is potentially something that a nice client could do for you but that shouldn't be relied upon. While aspnet model binding lets you play fast and loose with this a bit, it's still super normal to start your server side methods validating all the junk the client just sent you to make sure it's valid and reasonable and they are authorized to do it, etc. In a form post it's all string keys with string values.
Renerick commented 1 year ago

This is great, thank you so much!

danieljsummers commented 1 year ago

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.

Renerick commented 1 year ago

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