RevereCRE / relay-nextjs

⚡️ Relay integration for Next.js apps
https://reverecre.github.io/relay-nextjs/
MIT License
250 stars 30 forks source link

Handing GraphQL server errors #15

Open xuorig opened 3 years ago

xuorig commented 3 years ago

First of all thanks for this great package, super helpful ❤️

I'm trying to implement a redirect to login when my GraphQL server responds with a 401. One of the examples in this repo handles this by checking for valid auth before the GraphQL request which works but is not ideal given I'd need to make an auth check on every request.

It sounds like the best way to redirect on 401 would be to throw in the relay network/env and somehow catch it somewhere in getServerSideProps, so that we're able to return a nextjs redirect.

Unfortunately I don't think it's possible to handle this logic in userland through the getServerSideProps option this package provides, because that happens before loadQuery is called. https://github.com/RevereCRE/relay-nextjs/blob/main/src/wired/component.tsx#L81-L116

Is there a better way to do this? Happy to contribute something if needed 🙇

rrdelaney commented 3 years ago

Ah great question, we were facing the same thing. Our app has some public pages, some pages that require authentication, and some pages that require authorization. We couldn't find a general "Not authorized" solution we were happy with because:

What we ended up doing was letting Next.js take care of showing an error page when the GraphQL query failed to load data, and trying to not show users links to things they can't access.

xuorig commented 3 years ago

Thanks for your answer @rrdelaney! That's interesting. What about redirects that relate to domain logic, for example redirecting to an onboarding page if the user has not setup certain data? do you do it client side in this case? Cheers 🍺

rrdelaney commented 3 years ago

Ah yea in that case you can return an object from getServerSideProps and getClientSideProps that looks like:

{ redirect: '/get-started' }

Great question by the way, we should definitely add this to the docs.

xuorig commented 3 years ago

@rrdelaney just to be sure I understand correctly, I think the getServerSideProps that is passed to withRelay is executed before the query is made to the GraphQL server, meaning I can't act on the data received. I wonder if it's possible to support a use case like:

rrdelaney commented 3 years ago

Hm yea that one isn't possible at the moment. We don't expose a generic hook between "execute the query" and "render data" because that might be happening on either the client-side or the server-side. I think the main use-cases there would be showing 404 pages and redirects.

What you could do is execute the redirect in the network layer on the client and in getServerSideProps on the server. E.g if your API returns something indicating a redirect you can run window.location.href = ...

gio-work commented 2 years ago

To be certain, auth aside, what is the current approach for handling GraphQL errors?

For example, right now, we're redirected to a 500 page if a query fails. However, what if we wanted to be specific on what to do for different types of 500 errors?

What you could do is execute the redirect in the network layer on the client and in getServerSideProps on the server.

I'm unsure how this would help? If this cannot happen between "execute query" and "render data", then what options are we left with? Also, we're unable to use getServerSideProps while using withRelay. IE: You can not use getInitialProps with getServerSideProps.

For additional context, here's a solution I was considering (using the repo example as a reference)

// server_environment.ts
// ...
    const response = await fetch(apiHost, {
      ...requestVariables,
      body: JSON.stringify({
        query: params.text,
        variables,
      }),
    });

    const json = await response.json();

    // Interim solution for handling errors
    if(json.errors){
      return { data: { }, wiredErrors: json.errors }
    }

However, even if I get as far as to pass back errors silently, I still have nowhere to deal with this error, since wiredErrors will never make it to props, or anywhere I can use these values.

Any thoughts on this?

rrdelaney commented 2 years ago

In general, your GraphQL API should probably not be throwing errors for the queries needed to render a page. You can defer the parts likely to throw an error with a useLazyLoadQuery inside of it's own error boundary and handle those separately.

Re getServerSideProps: I meant the serverSideProps API that relay-nextjs provides, apologies for misstating,

gio-work commented 2 years ago

In general, your GraphQL API should probably not be throwing errors for the queries needed to render a page. You can defer the parts likely to throw an error with a useLazyLoadQuery inside of it's own error boundary and handle those separately.

Totally hear you on this. Maybe I can give you some examples and you can point me in the right direction in terms of best practice (knowing where this tool will go, has been, etc)? Appreciate your fast response and insight on this btw!

(less important: same as above, except for possible 500. although, to your point, this shouldn't happen)

Re getServerSideProps: I meant the serverSideProps API that relay-nextjs provides, apologies for misstating,

Ah, got it! Thanks, wasn't sure if I was missing something 🙏🏻

rrdelaney commented 2 years ago

Ah yea handling 404's like that can be tricky, I've had trouble doing it in our own application. We've been using @required(action: THROW) to denote that something should be there, and if not it's a 404. The issue is that error doesn't come up until rendering, and at that point it's too late to tell Next.js that there should be a 404 and render a 404 page. I'll update this issue if I find anything that works for us.

gio-work commented 2 years ago

Ah yea handling 404's like that can be tricky, I've had trouble doing it in our own application. We've been using @required(action: THROW) to denote that something should be there, and if not it's a 404. The issue is that error doesn't come up until rendering, and at that point it's too late to tell Next.js that there should be a 404 and render a 404 page. I'll update this issue if I find anything that works for us.

I wonder, is there a way to get the original requested route, params and query from 404? That may help a bit actually o_0 Just spitballing

FINDarkside commented 1 year ago

I've created a PR which allows to access query data and do redirects and set status codes based on that during SSR: #74

I think it'd be possible to modify so that you can return specific errors etc as well to be passed as prop, but I don't have a need for it at the moment so didn't really think about it.