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

@resolveError #80

Closed
wants to merge 8 commits into from
Closed

@resolveError #80

wants to merge 8 commits into from

Conversation

ericclemmons
Copy link
Owner

This is super-useful for catching bugs & stuff that happen during the @resolve...

@ericclemmons ericclemmons self-assigned this Sep 7, 2015
@ericclemmons ericclemmons added this to the v2 milestone Sep 7, 2015
@ericclemmons
Copy link
Owner Author

@goatslacker Check it:

errors

  • Fix issue where error (perhaps all data?) persists throughout history.

@ericclemmons
Copy link
Owner Author

Totally dig this.

error

@ericclemmons
Copy link
Owner Author

This is a pretty good example of what the v2 architecture tries to allow - "bubbling" of Promise handling up the chain.

Coming up next, #72!

@ericclemmons
Copy link
Owner Author

I've already been pinged to include <PrettyError>. I should be able to do it via react-resolver/components/PrettyError so it's optional.

@goatslacker
Copy link
Contributor

So nice

@monder
Copy link
Collaborator

monder commented Jul 26, 2016

@ericclemmons any chance to merge this?

@monder
Copy link
Collaborator

monder commented Jul 27, 2016

Aghh, just found out that it breaks server rendering. :) Need to think of some other approach

@ericclemmons
Copy link
Owner Author

Actually, we may want to revisit this from a clean slate. (Close this PR, basically)

I think the implementation of @loader (#39) but with a .catch clause is the way to go.

@monder You've been helping out a lot. Would you like contributor/publisher rights?

@monder
Copy link
Collaborator

monder commented Jul 28, 2016

Its not as simple as with @loader =) Tried that.
What I want to do is to have some sort of error handling that just works.
For example I could define:

<Component1>
  <ErrorHandlerX>
    <Component2>
      <ErrorHandlerY>
        <Component3>
        </Component3>
      </ErrorHandlerY>
     </Component2>
   </ErrorHandlerX>
</Component1>

Where ErrorHandlerY will catch errors from Component3
ErrorHandlerX will catch errors from Component2 and all unhandled ErrorHandlerY ones.
If none catches - the exception will bubble to server side catch.

That way for example I can draw a navigation bar on all 404 and 403 pages with the user's name.
And only when there was an error to retrieve the user context the rendered error will be without navigation bar =)

What I currently came up with and it works on both server side and client side - https://gist.github.com/monder/f3f969ca8210191b550af130627b26cf
(Requires #117)
But before I create a proper PR I want to fix some of the stuff I don't like (this['ReactResolver.IS_CLIENT'] for example, ResolveError extends Resolver, setState magic).

The api is

@resolveError((error) => {
  if (error.response && error.response.status === 404) {
    return error.response.status;
  }
  throw error;
}) 

So if the axios in my example throws an error in @resolve block and its a 404, the error property will be "404", else the error is passed to the next <Resolver> on stack if there is one. Why just "404" and not the whole Error object or ResolverError (which BTW is currently unused)? Its because the Error is not serialisable to JSON, and we require that to do the correct SSR hydration.

@ericclemmons
Copy link
Owner Author

Man, that sounds so slick Great writeup!

I'd suggest @handleError or @catchError since there isn't any resolution happening, but great work!

Let's get #117 taken care & get you access!

@ericclemmons
Copy link
Owner Author

Also, I think it'd be great to have an example for something like this, even if the error's faked or randomized between requests!

@monder
Copy link
Collaborator

monder commented Jul 29, 2016

@catchError sounds better indeed.
I will definitely add/update the example. But as you've already said it should be a new PR :)

@ericclemmons
Copy link
Owner Author

@monder Yep, you're right. Hook me up with your NPM name as well so I can give you publishing rights!

@monder
Copy link
Collaborator

monder commented Jul 29, 2016

Thanks. It's the same as on github - https://www.npmjs.com/~monder

@ericclemmons
Copy link
Owner Author

@monder You're added onto both npm and Github!

~/Projects/ericclemmons/react-resolver master ⇣
❯ npm owner ls
ericclemmons <[email protected]>
monder <[email protected]>
pwmckenna <[email protected]>

@ericclemmons
Copy link
Owner Author

Super old commits, so I'm closing due to inactivity. Any collaborator can re-open. If anyone else wants collab rights, ping me!

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.

3 participants