Error handling with async resolution

[ericclemmons]

Error handling with async resolution

[ericclemmons]

I normally use Promises (gasp!), but opted to support node-style callbacks for easy Isomorphic support in the examples.

Perhaps the question should be:

Who handles the errors & how?

I think you're right with preferring done(value) for callbacks, especially to integrate solve.

I very much like the idea of thrown errors (e.g. an ApiService gets a 500) being available for the UI to answer.

For example, suppose you're in a <Mail.Messages /> component that lazy-loads this.props.messages. If that fails, you'd want to render something like "Whoops! I couldn't get messages from the server. ?".

The question is, what in the component do we check to render this error?

Here's my proposal for the new API:

• Dynamic props should (somehow) have the equivalent propTypes checks.
  • If the prop is required (e.g. React.PropTypes.any.isRequired), then the render() is neutered (like it is currently) until the props are resolved
  • If the prop is optional (e.g. React.PropTypes.any), then it's expected the render() knows how to handle missing props by displaying Loading... or something.
• Dynamic props return a promise or call done(value), where this.props.myProp = value;.
• Any thrown errors will be caught by Resolver.mixin (or the this.context.resolver instance)
  • Errors could be available via this.context.resolver.errors.
  • The user could provide an onError callback for the new Resolver({ ... }) that gets called when any error occurs during resolution.
  • Errors could be handled via resolver.handle(Handler).catch(...) on the server.
  • Should errors be emitted for listeners to pick up on? (e.g. resolver.emit('error', { .. }))

As I created this project to encourage testability (by testing <Mail.Messages messages={...} /> on the server, and the client lazy-loading messages into <Mail.Messages />), the ideal solution would be something where we can test <Mail.Messages /> when it gets an error. Perhaps means mocking the failed service? But that seems like a coupling code-smell compared...

Back to you @jesseskinner!

[jesseskinner]

I like the idea of using propTypes to know when it's safe to render. Couldn't it use the actual propTypes on the component, rather than something that is the "equivalent of propTypes"?

This sounds good:

Dynamic props return a promise or call done(value), where this.props.myProp = value;.

For errors not caught within the statics definition, or that should be handled globally, I think this makes a lot of sense:

Errors could be handled via resolver.handle(Handler).catch(...) on the server.

If users want the errors to be available as props, they could use the mechanisms I described above to catch the errors as they happen, and use the done callback to expose them as part of the prop resolution.

So this would give users two ways to handle errors - inline (via callback or promises), and globally (via handler catch), and without adding new API to react-resolver. So the event handling of an onError callback is probably superfluous, because it can be done by the user themselves with code in the catch.

Regarding this suggestion:

Errors could be available via this.context.resolver.errors.

I think we should avoid introducing a third pathway for data, there are already props and state so having users access stuff on context is probably unnecessary. Also, I believe context is undocumented and might likely change in the future.

[jesseskinner]

Just had a thought. Callbacks could support both mechanisms via arguments.length, whether there is one argument - done(result) - or two - done(error, result).

[ericclemmons]

• Does throw new Error(...) get caught/wrapped by Bluebird?

[jesseskinner]

I haven't used Bluebird but the standard with Promises is that any errors thrown inside the promise handler or then callbacks will result in a failed promise and the error being passed to any catch callbacks.

[ericclemmons]

Note to self:

• Test cases for when errors are thrown
• Recommendation in the docs on how this is handled
• Discussion on if React.PropTypes.isRequired should be a pivot for bubbling up errors or trapping them

[ericclemmons]

Cleaning up issues. v2 is a complete re-write that's in production, and it's best to deal with any new architecture updates post-launch.

Still want to continue collaborating, and having seen performance issues with ES6 Promises, callbacks will be coming back!