node-osmosis icon indicating copy to clipboard operation
node-osmosis copied to clipboard

🙋FEEDBACK: Error Handling API

Open rchipka opened this issue 9 years ago • 9 comments

Currently, Osmosis only supports logging error messages.

This obviously isn't ideal since there is no way to programmatically identify error types without parsing a string.

I'm reaching out to Osmosis users for suggestions on how they would want this API to look and function based on how they use Osmosis.

Any thoughts/suggestions are welcome, especially:

  • "Let's name the command X"
  • "It'd be really cool if the API looked like X"
  • "I'm not sure how it'd be implemented, but I'd like it to do X"

Osmosis' error handling API implementation should include:

The ability to identify the source of the error, such as:

  • HTML/XML Parser
  • Socket
  • HTTP
  • DOM
  • Osmosis

The ability to identify the type of error, such as:

  • Parser: empty document
  • Parser: invalid document
  • Parser: libxml parse error
  • Socket: connect()
  • Socket: timeout
  • HTTP: redirect?
  • HTTP: protocol error (including status code)
  • DOM: <script> execution errors
  • Osmosis: no node(s) found
  • Osmosis: invalid API usage

The ability to identify fatal errors, critical errors, and warnings, where:

  • A fatal error is an error that causes the entire Osmosis instance to halt completely.
  • A critical error is an error that, if not caught and resolved, will not forward a context or data object to the next command in the chain.
  • A warning is an error that informs the user that something happened, but a context and data object will still be forwarded to the next command in the chain.

The ability to conditionally catch and "resolve" errors, for example:

  • Catch HTTP 404 and parse as a normal page.
  • Catch any HTTP redirects to '/login', complete the login process, and retry the request.
  • Catch a warning message, but instead choose not to continue down the chain.

rchipka avatar Nov 25 '16 18:11 rchipka

Ping @andineck @michaelhogg @bisubus @Ivanca @JonnyBGod @ImanMh @joefraley @thatisuday @cpietrzykowski @NegativeIQ @nashwaan @martyhu @MMRandy

rchipka avatar Nov 25 '16 18:11 rchipka

Thank you for picking this up. I made an attempt in the PR #90 which helped my at the time, but I see that it needs a more consistent error handling as you describe @rchipka .

I think Errors should be Identifiable with an Error code analog to node.js Errors.

I like the categorization into (fatal, critical, warning) as described above. What I think is really helpful that in the case of a warning, there is a context and data object, so that one can act upon it.

andineck avatar Nov 26 '16 14:11 andineck

@andineck I agree there should be context and data. It'd also be nice to have the HTTP response object.

I'm not sure how to implement the error handler so that it doesn't need a bunch of arguments (error object, response object, context object, data object, next function, done function). There must be a cleaner way.

rchipka avatar Nov 26 '16 15:11 rchipka

A possible solution could be to catch errors within .then() whenever you need a context.

.then(function (context, data) {
  this.on('error', function (err) {
    ...
  });
})

rchipka avatar Nov 26 '16 16:11 rchipka

Another possible idea: The error API could be a part of a larger event API.

This would allow you to listen for errors and other internal command events.

Some examples:

.follow('a')
.on('node', function (a) {
  if (a.href.charAt(0) !== '/') {
    return null;
  }

  return a;
})
.on('url', function (url) {
  return rewriteURL + url;
})
.on('response', function (res) {
  if (res.headers['content-type'].startsWith('image')) {
    saveImage(res.body);
    return false;
  }
})

Not perfect, but just a throwing it out there.

Note: How can we handle errors/events for the entire instance? Maybe if .on() is located at the top of the command chain?

rchipka avatar Nov 26 '16 16:11 rchipka

The ability to identify the source of the error, such as:

  • HTML/XML Parser
  • Socket (which end)
  • HTTP (which end)
  • DOM
  • Osmosis

I think it's important to know if the error occurs on creating the request (our side) or it's happening because of an internal server error or etc.

ImanMh avatar Nov 27 '16 08:11 ImanMh

@ImanMh Could you specify what you mean by "occurs on creating the request"? Do you mean an error caused by invalid input to the API (user tries to pass a buffer as a URL arg, etc.)?

In my opinion, Osmosis would be the source of those types of errors, which would imply that it happened on the client end.

rchipka avatar Nov 27 '16 16:11 rchipka

I think something like #90 is fine. It's really difficult to do error handling, and it seems weird to add control flow mechanisms into errors. I think for errors its probably enough to just examine the error object and let the osmosis library user code something about it.

If you are going to build control flow, I think doing something with events would be a neat way to do it. The good thing about doing it with events is that it's pretty conceptually easy to understand. You then get control flow in error handling for free.

martyhu avatar Nov 28 '16 00:11 martyhu

@rchipka Yea that is what I mean. I agree.

ImanMh avatar Nov 28 '16 17:11 ImanMh