Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug (?) in documentation on computed observables - writeable computeds don't have a chance to react to a change that is the same as current value #562

Merged
merged 2 commits into from

4 participants

@LinusKallberg

In the documentation on computed observables (http://knockoutjs.com/documentation/computedObservables.html), Example 3 illustrates how to wrap an observable in another computed observable to be able to validate values assigned to it and to prevent updates of the underlying observable in case of validation errors. In the example, the computed observable checks that the assigned value is a valid numeric value. However, after a validation error, if the input box is changed back to its last valid value, the error message is still shown. For example:

  1. Input "234". No error message is shown.
  2. Add an "a" to get "234a". The error message appears.
  3. Remove the "a" again. The error message still appears.

I have tested this in IE 9 and Chrome. I use Knockout version 2.1.0.

I tried setting breakpoints in both "read" (after wrapping it in a function) and "write", and they don't even seem to be called when the value of the input box is the same as the last valid value. Very strange; it seems that somehow, the current value of the internal observable is accessed without going through the computable observable.

I tried to use a similar solution for my own webpage, so it would be nice if it worked or if there was some simple workaround.

PS. My apologies if this has already been reported.

@rniemeyer
Collaborator

I think that the easiest "fix" for this particular scenario is just to force the written value to be numeric by doing + or parseFloat like:

    this.attemptedValue = ko.computed({
        read: this.acceptedNumericValue,
        write: function (value) {
            if (isNaN(value))
                this.lastInputWasValid(false);    
            else {
                this.lastInputWasValid(true);
                this.acceptedNumericValue(+value);
            }
        },
        owner: this
    }); 

However, this relies on the fact that the view model value is numeric and the new input is a string. If the validation in this scenario was different (maybe it could be checking the length of the string), then it would still fail.

This would have worked properly in KO 2.0. In KO 2.1, the writeValueToProperty function that was in jsonExpressionRewriting.js and now is in expressionRewriting.js, does a check to see if the values are different before writing. This means that the logic in the writeable computed observable will not have a chance to execute.

I think for something like the value binding (writes are driven by user input), the performance savings of the check are not likely worth much and we run into issues like this. I suppose that we could check if it is writeable and computed and then do the write, but we don't save much by not just writing the same value to a normal observable (as it will not notify).

@mbest
Collaborator

Obviously this is a scenario we didn't consider when we changed the value binding to only update if the value had changed. Considering that this is an example in the documentation, I think we should just change the value binding back to what it was and not worry about possibly updating the observable more than "needed".

@rniemeyer
Collaborator

I agree. I had definitely not considered this scenario with 2.1 and it seems like the right choice would be to match the previous functionality.

@LinusKallberg

Thank you for your fast responses! Until v2.2 is released, do you think I could just patch my copy of Knockout and set checkIfDifferent = false in the call to ko.jsonExpressionRewriting.writeValueToProperty() in the "value" binding? I tried it, and it seems to work, but maybe there is some subtle problems with this solution I haven't considered?

@mbest
Collaborator

Yes. That would be my fix as well.

@rniemeyer
Collaborator

This is ready to be merged. Change was just to not prevent writes from the value binding, even if the value is the same as the observable/computed's current value. This matches the pre 2.1 functionality. An observable will not notify on the same value anyways and as described in this issue we don't know what a writeable computed will do with the value that it receives. I added a spec that demonstrates the scenario that was causing an issue in 2.1.

@mbest
Collaborator

Looks good to me.

@SteveSanderson SteveSanderson merged commit d813528 into from
@SteveSanderson

Nice one - thanks!

@mbest
Collaborator

I've been thinking about this issue lately and have concluded that the pattern described in the documentation isn't a good one. The pattern uses a writable computed to filter updates to an observable (with a separate observable used to record the validity of the view data).

I think a much better pattern involves a simple computed observable to check for validity. This will work regardless of how the observable is set and doesn't cause a condition where the view and view-model are out of sync.

function MyViewModel() {
    this.numericValue = ko.observable('123');

    this.lastInputWasValid = ko.computed(function() {
        return !(isNaN(this.numericValue()));
    }, this);
}

http://jsfiddle.net/mbest/2eLS7/2/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 14, 2012
  1. @rniemeyer
  2. @rniemeyer

    Fix for #562, value binding should always write when triggered. (fix …

    rniemeyer authored
    …error in last minute variable name change).
This page is out of date. Refresh to see the latest.
View
37 spec/defaultBindings/valueBehaviors.js
@@ -43,6 +43,43 @@ describe('Binding: Value', {
value_of(myobservable()).should_be("some user-entered value");
},
+ 'For writeable observable values, should always write when triggered, even when value is the same': function () {
+ var validValue = ko.observable(123);
+ var isValid = ko.observable(true);
+ var valueForEditing = ko.computed({
+ read: validValue,
+ write: function(newValue) {
+ if (!isNaN(newValue)) {
+ isValid(true);
+ validValue(newValue);
+ } else {
+ isValid(false);
+ }
+ }
+ });
+
+ testNode.innerHTML = "<input data-bind='value: valueForEditing' />";
+ ko.applyBindings({ valueForEditing: valueForEditing}, testNode);
+
+ //set initial valid value
+ testNode.childNodes[0].value = "1234";
+ ko.utils.triggerEvent(testNode.childNodes[0], "change");
+ value_of(validValue()).should_be("1234");
+ value_of(isValid()).should_be(true);
+
+ //set to an invalid value
+ testNode.childNodes[0].value = "1234a";
+ ko.utils.triggerEvent(testNode.childNodes[0], "change");
+ value_of(validValue()).should_be("1234");
+ value_of(isValid()).should_be(false);
+
+ //set to a valid value where the current value of the writeable computed is the same as the written value
+ testNode.childNodes[0].value = "1234";
+ ko.utils.triggerEvent(testNode.childNodes[0], "change");
+ value_of(validValue()).should_be("1234");
+ value_of(isValid()).should_be(true);
+ },
+
'For non-observable property values, should catch the node\'s onchange and write values back to the property': function () {
var model = { modelProperty123: 456 };
testNode.innerHTML = "<input data-bind='value: modelProperty123' />";
View
2  src/binding/defaultBindings/value.js
@@ -13,7 +13,7 @@ ko.bindingHandlers['value'] = {
var valueUpdateHandler = function() {
var modelValue = valueAccessor();
var elementValue = ko.selectExtensions.readValue(element);
- ko.expressionRewriting.writeValueToProperty(modelValue, allBindingsAccessor, 'value', elementValue, /* checkIfDifferent: */ true);
+ ko.expressionRewriting.writeValueToProperty(modelValue, allBindingsAccessor, 'value', elementValue);
}
// Workaround for https://github.com/SteveSanderson/knockout/issues/122
Something went wrong with that request. Please try again.