Creates ko.onError handler

[harry8525]

This creates a optional ko.onError callback property that will be called on new async callstacks when errors are hit. This allows us to get usable callstacks in browsers that do not have the stack param on the window.onerror event (e.g. IE 11 and lower).

[brianmhunt]

Many thanks @harry8525 for the patch. I really think this deserves some attention since error handling is something that could use some ❤️.

Just a side note, @harry8525 I noticed a number of places in the PR that are purely stylistic/whitespace and they make it harder to see whether the changes are substantive. Is there an easy way on your end to back out the non-semantic (i.e. purely-stylistic/whitespace) changes?

Also, I am noting #1628 and #1659, since they might make it in before this PR and are affected/related.

Cheers.

[SteveSanderson]

I'd like to merge this one soon. I'll take care of squashing it into a single commit and fixing the whitespace issues.

Any objections? @brianmhunt - if you think anything about this doesn't fit with #1628 or #1659, please let me know.

[brianmhunt]

Thanks @SteveSanderson ; this would be great to have.

I have one concern, being if ko.onError only takes a string argument. For two reasons, below, I feel like the signature ought to be closer to the one in #1628, i.e. ko.onError taking a single object-argument, with a set of conventional properties e.g. {errorCaptured: err, during: "setTimeout"}.

The first reason is to have a single error handler for the framework, so that it becomes simple for users to capture errors with a predictable convention from every place Knockout might throw them. The second is to future-proof the error handler, in case someone wishes to expand it in future (i.e. by passing more properties).

The difference to accommodate this concern is trivial, and would be at https://github.com/knockout/knockout/pull/1715/files#diff-2b4ca49d4bb0a774c4d4c1672d7aa781R343 where it would become:

ko.onError && ko.onError({errorCaptured: e, during: "setTimeout"});

Users may have to watch the during as we expand to capture errors from other places.

The #1659 patch should not end up requiring much workaround (though the during argument may be properly changed with #1659 from setTimeout to e.g. animationFrame).

Does that make sense? Of course, I leave it to you to go either way, but wanted to cover the bases. 😄

[SteveSanderson]

It's definitely worth thinking through the futureproofness.

In this case it looks to me like it will already be OK. The argument passed to onError is the error object, which in the case of a throw new Error or a typical null-ref exception or similar will be an Error instance containing not just the message but also stack and other information. That covers the majority of the use case for additional during information. If in the future we want to pass additional KO-specific information, that can be supplied as a second parameter to onError without it being a breaking change.

I don't have a big problem with wrapping the Error object in some KO-specific container, but it doesn't really seem necessary to me (given that we can always add additional args if a need arises). If you feel strongly that we should wrap it please let me know!

[harry8525]

I'm not sure how well the all browsers handle rethrowing custom errors so we should do some tests if we want to go down that path. Aka not sure if the stack is still preserved on rethrow (it should work but you know browsers :)).

I agree with Steve that adding extra metadata as additional args seems cleaner to me. RequireJs appends things to the error object but that complicates serialization because sometimes you get weird things like elements and other stuff you were not expecting depending on the error.

[brianmhunt]

Thanks for the comments. I have nigglies, but I am not sure they ought to be persuasive. Let me write them out, and we will see if there is any substance to them.

Instead of wrapping errors one might have custom Error constructors, and maintain the pattern of only-one argument (more on that below). For ease of reference, custom error constructors would probably look a bit like this:

function BindingError(spec) {
  // Error mucking – see http://stackoverflow.com/a/17891099/19212
  this.during = "binding";
  this.node = spec.node;
  this.bindingContext = spec.bindingContext;
  // ...
}
BindingError.prototype = Error.prototype;

The problem with a Error constructor is that for decision making by error handlers the during property is now conflated with the Error constructor class (should users test the type of error by asking during === ? or error instanceof ?? It is less of a hack to demultiplex on during so that feels superior from that perspective); comparing during is also better for decoupling. Indeed, wrapping errors (as opposed to extending them) would seem to result in looser coupling in general. KO could have a generic KnockoutError, too, but then some finagling might have to happen to get the error-instance properties to co-operate for each permutation (i.e. binding errors and setTimeout errors shall have a whole set of mutually exclusive properties that may be useful to error handlers).

I have a niggly about adding an additional argument. I cannot put my finger on it, and perhaps it is only because I have built up a personal bias towards the Promises pattern of do.then(something).catch(ko.onError). More generically, I would think it would be valuable to avoid any possible variant of the boolean trap. I also recall something from Douglas Crockford that comes to mind, if I have it right (which I probably don't): in cases like this, for extensibility and future-proofing, a function ought to accept one argument: an object. This pattern also happens to fit well with ES6/ES.next destructuring too, and I personally have had good dividends with little drawback with this line of design.

I am not sure these are persuasive or compelling reasons to consider a custom wrapper or otherwise, and I am completely content with any outcome here, but having written it out my sense is that a wrapper has the best “smell” of all the options.

[harry8525]

So are we waiting on other commits or should I fix up the whitespace now?

I just want to get this in so we can catch all our errors :)

[brianmhunt]

😁 @harry8525 – It's in the hands of @SteveSanderson now — I hope I have not derailed this with my chatter above:grey_exclamation:

[SteveSanderson]

OK, I've got this into a shape where I'd be happy to merge it: See #1751 (which supersedes this pull request we are commenting on here)

@brianmhunt, about the API and whether errors should be wrapped, this is not something I'm personally concerned about since this approach passes through the original Error instance so it can come with whatever metadata it needs (and already comes with a stack trace by default). However, if you do want to change this, would you mind adding to the pull request?

I'd like to merge this soon if possible.

[brianmhunt]

@SteveSanderson – Let's merge it.

As long as the error argument is an object or Error instance (i.e. not a string), I'm sure this'll do.

Thank you!

[SteveSanderson]

Thanks - merged!

[SteveSanderson]

Whoops - unmerged. I see there's still code review feedback to account for.

[SteveSanderson]

... and now re-merged, since I see the code review feedback concludes that there's nothing to change.

[brianmhunt]

:) Excellent.