Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deferred updates, part 2: default option #1753

Merged
merged 3 commits into from Apr 15, 2015
Merged

Conversation

mbest
Copy link
Member

@mbest mbest commented Apr 8, 2015

After reading @brianmhunt's comment on #1603, I thought of using the same API for controlling whether to use global deferred updates. But I like ko.options instead of ko.settings, so we have ko.options.deferUpdates.

Following the information I presented in #1728, this also includes a method to run pending tasks immediately. Instead of ko.tasks.runTasks, the name I had used in #1738, I decided on ko.tasks.runEarly, which I think makes its purpose more clear.

@brianmhunt
Copy link
Member

I like both ko.tasks.runEarly and ko.options.

@mbest mbest added this to the 3.4.0 milestone Apr 8, 2015
@IanYates
Copy link

IanYates commented Apr 9, 2015

👍 - The new naming makes the intent more clear

expect(runValues).toEqual([1,2,3]);
});

it('Should keep correct state if a task throws an exception', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that runEarly should throw if one of the handlers it runs throws. This means everywhere you use runEarly, you have to put all your code in try/catch, because you have no idea whether some completely unrelated task will throw. And people can use it as a way of detecting whether or not at least one scheduled handler does throw (without telling you which handler), which encourages excessive use of runEarly, undermining the benefit.

Could we change this so runEarly doesn't throw? Throwing doesn't seem to communicate anything useful, as you have no idea what threw (and I don't think we should add a way of reporting what threw, as it would lead to really weird use cases). Then all task exceptions are definitely going to land in the same place - the onError callback (or window.onerror if you catch at that level).

@mbest
Copy link
Member Author

mbest commented Apr 10, 2015

@SteveSanderson I'm glad you brought up exception handling since it was something that I felt a bit unsure about.

The main advantage of falling out of the task loop on an exception is that the error gets reported right away in the correct context. We already have somewhat of a precedence for this because if you update an observable, it can trigger any number of manual subscriptions, computed observables, and bindings to run, and errors will both cancel the remaining updates and get thrown back to the caller (the code updating the observable).

I'm also not too concerned about the remaining tasks being rescheduled after an exception, since if using MutationObserver or onreadystatechange, it still happens before the UI is redrawn.

But, I do agree with your concerns about errors when using runEarly. At least when using runEarly, we can handle the errors asynchronously. I've seen this done before with something like this:

try {
    //...
} catch (e) {
    setTimeout(function() { throw e; }, 0);
}

…or all observables

ko.tasks.runEarly runs all pending tasks immediately

ko.tasks.reset ensures tests leave a clean slate
…e queue get run.

Rename ko.tasks.reset to ko.tasks.resetForTesting so that its intent is more clear.
@mbest
Copy link
Member Author

mbest commented Apr 11, 2015

After trying a couple of methods, I decided that the simplest and most clear way is to have all task errors deferred using setTimeout. So this should now work as you've suggested, @SteveSanderson.

@mbest
Copy link
Member Author

mbest commented Apr 15, 2015

I'm merging this now so we can move on to the next part.

mbest added a commit that referenced this pull request Apr 15, 2015
Deferred updates, part 2: default option
@mbest mbest merged commit 1dbe749 into master Apr 15, 2015
@mbest mbest deleted the deferred-updates-part-2 branch April 15, 2015 23:13
@rniemeyer rniemeyer mentioned this pull request Aug 30, 2015
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants