CheckedObserver truthy values and matcher coercion

[fkleuver]

In fixing the CheckedObserver tests / logic for the SubscriberCollection PR I noticed a few things that might be up for improvement, but in order to keep that PR clean I'm adding some notes here instead.

If you set the value property of a checkbox to a non-string primitive and have the checked property bound to an array with that same non-string primitive, the corresponding input element would not be checked because the value property of a checkbox is always coerced to a string. Users might consider this unintuitive.

In the default matcher function:

(a, b) => a === b

We could coerce the value (a, which comes from the VM) to strings:

(a, b) => (a+'') === b

This would make the behavior consistent with how HTML behaves.
The performance impact here would be negligible since adding an empty string to a string is usually optimized by browser jit.

Furthermore, for similar reasons we could consider being more loose with the checked property comparison. Booleans have a tendency to get coerced to strings somewhere along the flow of binding.
So we might want to change this:

...
if (value === true) {
      element.checked = true;
...

To this:

...
if (value === true || value === 'true') {
      element.checked = true;
...

@EisenbergEffect @bigopon

[EisenbergEffect]

The looser comparison seems like it might be the way to go, rather than coercing values.

[fkleuver]

Can you elaborate?

You mean a == b instead of (a+'') === b or?

[EisenbergEffect]

I meant this value === true || value === 'true'. Or is this two separate issues?

[fkleuver]

Yeah it's two separate ones. (a, b) => a === b is the default matcher function for radio input collections and checked arrays. value === true is for single primitive values

The primitive only responds to boolean at the moment. But since checked only responds to boolean, coercion numbers wouldn't make sense anyway, hence the || value === 'true'

The array is a bit broader because it simply looks for equality with the .value attribute, hence the a+''

[EisenbergEffect]

We should get @jdanyow to provide feedback on this. I thought the model properties we set up handled keeping the original types. It was a long time ago that we addressed this issue though.

[fkleuver]

Yep just ran into this with the unit tests. The model property doesn't have this problem so I would only apply the coercion logic for the value attribute. I've always seen the model as a way to support objects, not necessarily for the coercion.
It's possible to solve the coercion with model, but letting that be the only way seems counter-intuitive.

For example:

input.value = 42;
input.value === 42; // false - would become true

input.value = '42';
input.value === 42; // false - would become true

input.value = 42;
input.value === '42'; // true - stays true

input.value = 'true';
input.value === true; // false - would become true

etc..

[bigopon]

Just walked through the comments, I'm not sure how to look at this. Though altering behavior of input.value could be nice, I don't think it's a good idea as it's no longer predictable. That's for our core implementation. In the future, we can allow observer locator to be override-able / extendable and can experiment with all these maybe intuitive behaviors.

[fkleuver]

It's the other way around. Currently it's not predictable because the DOM will coerce values but the observer won't. I'm simply suggesting we should make the observer do the same coercions that the DOM does, so that it becomes predictable :)

[davismj]

Is there any reason we have to use the input.value property for these comparisons? Couldn't we handle all cases the same way we handle model.bind?

[bigopon]

Implicitly converting the value types could be double edged, we better wait until the first request comes to have some concrete case based ideas.