BorisMoore / jsviews

Interactive data-driven views, MVVM and MVP, built on top of JsRender templates
http://www.jsviews.com/#jsviews
MIT License
856 stars 130 forks source link

radiogroup tag and direct data-linking behave differently when using numeric model values #361

Closed trueqbit closed 7 years ago

trueqbit commented 7 years ago

I am surprised that jsviews' radiogroup tag behaves differently from directly binding to input elements if the data model contains numeric values. Specifically the tag version doesn't check any radio button while the direct binding does.

Outline:

<script id="#tag-template" type="text/x-jsrender">
  <input type="radio" value="1">
</script>
<script id="#direct-template" type="text/x-jsrender">
  <input type="radio" value="1" name="type" data-link="type">
</script>

$.templates('#tag-template').link('#tag-target', { type: 1 });
$.templates('#direct-template').link('#direct-target', { type: 1 });

See also fiddle.

Paul-Martin commented 7 years ago

I've run into this as well. What I realized was that technically input values are strings. So changing your example to:

$.templates('#tag-template').link('#tag-target', { type: '1' });
$.templates('#direct-template').link('#direct-target', { type: '1' });

produces the same behavior for both. I haven't check the code, but I suspect in the second case it is being coerced to a string and not the first. If you can't change property 'type' to string, you could use a converter function to convert them to/from strings

$.views.converters({
        intToStr: function(value) {
            return value ? "" + value : "";
        },
        strToInt: function(value) {
            var notNumeric = isNaN(value);
            return notNumeric || value === '' ? null : parseInt(value);
        },
})

{^{radiogroup convert='intToStr' convertBack='strToInt' type}}
...
{{/radiogroup}}

<input type="radio" value="2" name="type" data-link="{intToStr:type:strToInt}">

See updated fiddle for converter example https://jsfiddle.net/5n4xes0a/1/

BorisMoore commented 7 years ago

Yes, correct, there is a difference between the two, concerning type coercion to string.

{{radiogroup}}: https://github.com/BorisMoore/jsviews/blob/master/jsviews.js#L5178 linkedEl[CHECKED] = (linkedEl.value === val); (Not coerced)

direct linking: https://github.com/BorisMoore/jsviews/blob/master/jsviews.js#L3827 sourceValue = target.value === ("" + sourceValue); (Coerced)

It would be better to have the same behavior for both. Technically, non-coerced is more correct. In the coerced behavior, the value of type will change from 1 to "1" if you switch to another radio button and back to the first. So returning to the same state from a user point of view (first button checked) is not returning to the same state internally. (Different data).

If you include in the template {^{:type + 1}}, then you see it starts out rendering as 2 and then renders as 11 (actually "11") on toggling back and forth the radio button. (See https://jsfiddle.net/5n4xes0a/2/)

So I am tempted to remove the coercion in the direct case - which will get consistency, avoid hidden side-effects (type change) on the data, and will in fact force people to use converters, or change their data. But it is a breaking change.

The alternative would of course be to add string coercion to the {{radiogroup}} case. The main advantage of that is it brings consistency without being (in practice) a breaking change. But I don't like adding the hidden side effect... Hmmm...

Paul-Martin commented 7 years ago

I would vote for the breaking change. It took me a while to figure out a bug I was having when sending data back to the server because of the datatype being changed.

trueqbit commented 7 years ago

@Paul-Martin Thanks for pointing out the converter option, which I wasn't aware of. @BorisMoore Thanks for your explanations. The (non-)coercing should be documented regardless of which way we/you choose. Either way might not be obvious and it's good to read about it somewhere.

At first thought, coercing to string has the advantage that it's doing the naïve but intuitively right thing for simple cases.

Nevertheless I vote for the breaking change as well. It's not only technical correctness, but it solves the inadvertent changes to the underlying data type, which in turn lead to subtle bugs, as Paul-Martin points out. I consider the side-effects worse than being forced to be explicit. And it's far cleaner to make a correction sooner than keeping side-effects, which bite and only need explanations for the rest of the days to come.

BorisMoore commented 7 years ago

Yes, I agree. I'm glad that you both have the same take on it that I do, in favor of correctness/no side effects.

BTW there are examples here http://www.jsviews.com/#samples/form-els/converters and here http://www.jsviews.com/#samples/form-els/array-binding of doing the conversion using integers. I'll try also to document the non-coercion elsewhere - probably in this section: http://www.jsviews.com/#link-input@radio.

BorisMoore commented 7 years ago

Fixed with commit 84