Set value on SetterObservable when it's set

[RyanMilligan]

In migrating one of our applications to Can 4, I've encountered what appears to be a pretty low-level problem in SetterObservable: specifically, when you call its setter, it never actually updates its value property. I assume this is because it's expected that the observation handler will be used to retrieve the value, but when can-bind calls the setter in updateValue (args.observable will be a SetterObservable in the case where value that's changing is bound to a converter), it then immediately calls the getter on the same observable.

This would be fine, except that because bound is true, the getter (implemented in get-set.js) skips using the observation function and returns this.value directly, which returns whatever the parent value was at the time the binding was set up.

This has a few negative side effects when using two-way binding through a converter:

1. A confusing semaphore warning is written out to the console because the updateValue function referenced above will cycle multiple times trying to get the parent and child values to agree (even though they already do; it's simply looking at an old snapshot of the parent value to do the comparison), which involves overwriting the child value with the original value of the parent, which will basically always be wrong
2. The issue that led to me debugging this problem was that my child value was being set first to the expected value, then to whatever the parent value was when the UI was created, then back to the correct value, which was causing unexpected behavior in my application.

I've created a jsbin that reproduces my binding scenario and demonstrates the warning: https://jsbin.com/kevolovonu/4/edit?html,js,console,output

I was unable to get it to reproduce the multiple sets, but I think the warning is probably sufficient to demonstrate that there's a problem here. The solution I found was to add this.value = to the beginning of SetterObservable.prototype.set.

[RyanMilligan]

Update: I found another scenario where I was still getting extraneous set calls on my view model, this time from the correct value to the same correct value again. I was able to fix it by checking if the new value matches the current value before calling the setter. The full fix now looks like this:

SetterObservable.prototype.set = function(newVal) {
	if(this.value !== newVal) {
		this.value = this.setter(newVal);
	}
};

[justinbmeyer]

What an awesome bug report! Thanks. I hope to get to this tomorrow. If not, please remind me Monday.

[justinbmeyer]

BTW, you could do this:

    test: {
      default: () => { return {} }
      Type: {
        timesSet: {default: 0},
        foo: { ... }
      }
   }

Essentially, the default empty object will be used to create an instance of the inline Type definition.

[justinbmeyer]

@RyanMilligan what warning are you expecting to see?

I'm confused a bit by the example, which I've made small changes to here:

https://justinbmeyer.jsbin.com/wecija/1/edit?html,js,console,output

<home-page>'s test is a DefineMap, which is passed to <sub-component> through a converter, but subProp should still be an object:

 <sub-component subProp:bind="myConverter(test)" />

So subProp's getter will make that object NaN:

    subProp: {
      get(val) {
        console.log(val);
        return parseInt(val);
      }
    }

This is why the input element is NaN.

Is this expected?

[justinbmeyer]

@RyanMilligan I get the warning now. You expected to see it on a change of the input element. I'm going to see if I can produce it with something a bit closer to what I think your code might look like (getting passed a number instead of an object).

[RyanMilligan]

Sorry, I should have been clearer; yes, the warning appears when you change the textbox. And also, yes, the whole thing with the converter and the object came out weird because I was trying to reproduce the larger scenario. I should have gone back and tidied it up before submitting but I'd already spent quite a lot of time debugging the issue to begin with.

Thanks for taking the time to untangle it.

[justinbmeyer]

oddly, when I update the example to something that seems to make more sense, the problem goes away: https://justinbmeyer.jsbin.com/wecija/2/edit?js,console,output

Part of the issue I'm struggling with, is that setting the SetterObservable MUST currently update something the observation reads. By doing that, this.value should be updated here (within Settable's inherited code).

I feel like value shouldn't be set by actually calling set. It's more-or-less a connivence.

So, I could use an example that more or looks like the way something "should" be written. I'm going to keep experimenting, but if you have ideas / thoughts, that would help. Thanks!

[justinbmeyer]

Here's another example that produces the warning right away, but not when the input changes:

https://justinbmeyer.jsbin.com/sofahel/2/edit?html,js,console,output

I'm going to try to write out what happens:

1. <sub-component subProp:bind="myConverter(test)" /> is run, this will output:

   myConverter.get DefineMap

2. value:bind="subProp" is run, this will lookup subProp, which ends up doing val.foo, which
   will log:

   foo.get undefined

   In subProp's getter, it logs:

   subProp.get obj undefined

Now the big question is why does it log:

subProp.get

again?

It seems it does this because after setting up observables for both sides of subProp:bind="myConverter(test)", it then tries to make sure the values match. They don't because subProp is undefined, and myConverter() is returning an object. But new Bind() initialized not to care here.

Eventually, however, value:bind="subProp" is initialized. This will try to set subProp to "". Hence the:

subProp.get

This will make subProp a NaN. Which will run the converter:

myConverter.set NaN

Which will run:

foo.set NaN

This will re-run the getter:

foo.get NaN

Since val.foo is still observable within subProp (I'm not sure why this is), subProp.get re-evaluates, which logs:

subProp.get obj NaN

Finally, the log is present because the child of value:bind="subProp" is "NaN", but the parent (subprop) is NaN. This behavior is right and I'd argue it shouldn't be changed.

I'm going to try to do a similar run down for your example.

[justinbmeyer]

In this case which is identical to your situation: https://justinbmeyer.jsbin.com/rinipuk/1/edit?js,output

After changing the input to 1, it logs:

subProp.get  1
myConverter.set  1
foo.set  1
subProp.get  DefineMap
runner-4.1.4.min.js:1 can-bind updateValue: attempting to update child AttributeObservable<input.value> to new value: NaN
…but the child semaphore is at 1 and the parent semaphore is at 1. The number of allowed updates is 1.
The child value will remain unchanged; it’s currently: "1"

When the input changes to 1, scopeProp is going to be set to "1". This is what logs:

subProp.get 1

That change is going to fire off the converter's setter, logging:

myConverter.set 1

That sets the foo property, logging:

foo.set 1

The real question here is why does subProp.get DefineMap get logged. ???

It seems that when subProp.get changed to 1, the following happens (can.queues.logStack()):

SubComponentVM{}'s subProp event emitter 
Observation<viewModel.subProp>.onDependencyChange 
Observation<viewModel.subProp>.update
SetterObservable<viewModel.subProp>.handler 
update scope.myConverter(test) of <sub-component> 
Observation<SubComponentVM{}'s subProp getter::SettableObservable>.onDependencyChange 
Observation<SubComponentVM{}'s subProp getter::SettableObservable>.update

update scope.myConverter(test) of <sub-component> makes sense ... we have to update a value through the converter. But what are these 2:

Observation<SubComponentVM{}'s subProp getter::SettableObservable>.onDependencyChange 
Observation<SubComponentVM{}'s subProp getter::SettableObservable>.update

So this means that subProp's internal "set" value changed from "1" to a DefineMap.

This is due to the TWO two-way bindings. <input value:bind is setting it to "1", subProp:bind="myConverter(test)" will set it to a DefineMap.

Ah ...

I think I identified the problem. As subProp:bind="myConverter(test)" is a "sticky" binding, it will try to set the SetterObservable represented by myConverter(test) to 1. But since it is sticky, it will then try to make sure the values match after doing this.

It will then read the SetterObservable again, see that its value is DefineMap, and set subProp to the DefineMap again ... this logs:

subProp.get DefineMap

Which changes the value back to NaN.

[justinbmeyer]

@RyanMilligan so in summary, I'm not sure what to change here. In my opinion, SetterObservable is doing the right thing. Also, we should be warning in this case.

I see a few options:

• Give people a way to "turn off" sticky bindings. If CanJS's bindings were not sticky, it wouldn't be trying to force subProp to stick to the DefineMap. Something like <input value:loseBind="subProp"/> If this is something you'd like us to pursue, I need to understand the use case better.
• Update your code to make sure that values don't create cycles. This can be done by ensuring that values will be === across two-way bindings. For example, the converter returns an object, but is setting a number property.
• We should improve the warnings to explain what is going on better. Capturing the queues stack would help.
• We should improve the docs around converters and bindings, helping people understand how they work and their stickiness.

[RyanMilligan]

Hi @justinbmeyer , sorry I haven't been monitoring this! I don't seem to be getting any notifications from GitHub when I get mentioned and I just forgot to check back.

I think there's still something missing from the repro that we have in our real scenario, because I'm seeing it compare the "new" value to the original value that the parent was originally initialized to, and then setting the child to that for no good reason. The warning comes up either then (which is relatively ok because we don't want it set to that anyway) or when it tries to change it back (which is not ok, because then it gets stuck on the old, incorrect value).

The end result that I'm seeing is that if I make a selection in my <select> element, the property I have bound to the value gets set first to the correct value, then to undefined (or null if I initialize the property to that), and then back to the correct value.

I think a live debugging session might be the best way for me to show you what's going on. I also have a separate issue in a prototype I'm working on that I'd like to run past you as well.