Knockout-Validation icon indicating copy to clipboard operation
Knockout-Validation copied to clipboard

Async result still applied even when condition changed to false

Open lostincomputer opened this issue 9 years ago • 11 comments

The result of async rule is still being applied even when the condition has changed to false. This causes Knockout Validation to mark the observable as invalid even when it should be valid.

Timeline:

  1. Observable is set to an invalid value.
  2. Rule's condition is set to true.
  3. Knockout Validation runs the async rule.
  4. Rule's condition is changed to false
  5. Async rule finishes and calls callback(false)
  6. observables.isValid is false when it should be true!

Same problem occurs if the observable's value changes before the async rule finishes.

lostincomputer avatar Mar 03 '15 10:03 lostincomputer

Hi there,

I'm not sure, but I think I'm facing similar issue.

In my case the problem is that the last async validation fired does not cancel off the previous one, which yields an invalid validation state in some cases.

I've written a small example to demonstrate it: http://jsfiddle.net/8LxL9u9v/9/

Just type "TT" and then, on quick sucession "TF" and "TT".

The final state should be valid, but instead it is marked as invalid because the last async validation takes less time to compute.

Thanks

ncortines avatar Mar 04 '15 16:03 ncortines

The condition is used only to determine if the validation should run, not to set the validation result. This can be overcome by watching isValidating property of the observable and prevent changes that would cause re-validation. Also, check the jsFiddle example bellow.

@ncortines I've modified the example to delay validation when you type in the input. The control is also changed to be read-only when validation is active. The example also makes use of throttle option to delay the validation.

http://jsfiddle.net/rs817sw8/

crissdev avatar Mar 04 '15 18:03 crissdev

Unfortunately, our application is required to not block the user when validation is running. We can't set the control to readonly or disable (or the users and BA will get mad).

lostincomputer avatar Mar 04 '15 19:03 lostincomputer

@crissdev , thanks for the example, it is a good workaround.

However I have to agree with @lostincomputer . The current behavior does not seem to be correct.

ncortines avatar Mar 04 '15 19:03 ncortines

@crissdev I forgot to mention that our async rule uses a debouncer (delay) before it calls the server but it gets defeated when the network or server is too slow. It's rare but it does happen.

lostincomputer avatar Mar 04 '15 20:03 lostincomputer

Thanks for your input. Async validation can really be a pain sometimes. But we can try to make it better :smiley:

Considering to give the last ran validation the final word on the result would imply to discard any previous callback results until the last pending operation completes. I hope I'll explain this well and provide some example tomorrow. This could be achieved by storing a token for each async validation and checking it when the operation completes. If the token was not created by the running operation then it should be discarded, until the last operation completes. This means that if 5 validations are running then only the last one should affect the validation state.

To summarize:

  1. Observable changes 2 times causing 2 async validations (op1 and op2) 1.1 Each async validation call creates a token and stores it on the observable (e.g. token)
  2. op1 completes but the token is different :arrow_right: discard result
  3. op2 completes and the token is the same the operation had set :arrow_right: update validation state

Any words on this?

crissdev avatar Mar 04 '15 21:03 crissdev

In my opinion discarding the op1 result while op2 is still in progress is wasting valuable information, since it informs the user about the latest valid state.

I would make this token a variable which is incremented per each validation fired. When the callback is finally executed with the validation result, it should check whether the new state is valid by comparing the token with the token that successfully updated the state for the last time:

  1. op1 is fired with token 1
  2. op2 is fired with token 2
  3. op3 is fired with token 3
  4. op1 resolves and updates the state. At this point op2 and op3 have not resolved yet.
  5. op3 resolves and updates the state (3 > 1). At this point op2 has not resolved yet.
  6. op2 resolves but does not update the state because it's token is not valid (2 < 3)

ncortines avatar Mar 04 '15 22:03 ncortines

Showing stale validation state can mislead the user though unless the UI has an 'is validating' indicator.

By the way, isValidating property doesn't support concurrent async operations. The easiest way to fix this is to add a counter that is incremented when an async operation begins and decremented when an async operation finishes.

isValidating can then be converted to a computed property:

isValidating = ko.computed(function() { return counter() > 0; });

lostincomputer avatar Mar 05 '15 06:03 lostincomputer

You are right. Intermediate results could be safely discarded. Valid unless proven invalid. I'm fine with the solution described above by @crissdev

ncortines avatar Mar 05 '15 09:03 ncortines

There are two reasons for isValidating to be updated. First: multiple async rules attached to the observable. Second: the async rule is still running when it is called again.

The counter will solve the first case by keeping track of the number of running async rules.

Alternatively, if we want isValidating to indicate that an async operation is running even if the result will be discarded, then the counter will just count all async operations instead of running async rules.

P.S. We don't attached multiple async rules in our observables but if other people will or does, modifying isValidating will be great.

lostincomputer avatar Mar 05 '15 09:03 lostincomputer

Thank you guys for your help on this.

Using some random string or a counter will both help in keeping the isValidating property consistent. The advantage of using a counter is it helps when debugging.

@lostincomputer There's no need to make isValidating a computed property because we might need to manually set that property in the future.

crissdev avatar Mar 05 '15 10:03 crissdev