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

Support redirects in getServerSideProps #13470

Closed
wants to merge 6 commits into from

Conversation

justincy
Copy link
Contributor

@justincy justincy commented May 27, 2020

This PR recommends adding support for redirects in getServerSideProps, as requested, like this:

export async function getServerSideProps() {
  return {
    props: {},
    redirect: '/',
  }
}

There is an existing pattern for redirects but it has some problems. SSR requests work as expected because the browser is requesting HTML, follows the redirect, and receives HTML. But data requests fail because the browser is requesting JSON but receives HTML. This causes an error which is sometimes caught by Next which triggers a full page reload, giving the appearance of it working properly, but this fails in Safari.

The pattern proposed here avoids the problems stated above with data requests while maintaining the same behavior for SSR requests. A similar pattern has been proposed for custom status codes and error handling.

I don't know whether this is the best way to solve the problem. I'm sure you all have some idea of how to expand the functionality of getServerSideProps. I look forward to the discussion.

If this seems like an acceptable solution, there are some details that need to be addressed:

  • Should we allow for custom response codes in the redirect?
  • Should we disallow both redirect and props to be used at the same time?
  • Do we need to allow for redirect to support both as and href?

@fabb
Copy link
Contributor

fabb commented May 28, 2020

Should we allow for custom response codes in the redirect?

I’d say definitely yes, since the response code (301 or 302) makes a lot of difference for SEO. We use both in our application currently, depending on semantics of the redirect.

@timneutkens
Copy link
Member

This should probably be a RFC first given that it's unclear what the correct behavior is. It's been on my to-do list. Here's some of the requirements:

  • Has to work for both getStaticProps and getServerSideProps
  • Has to allow returning permanent true/false (default of not permanent) (no custom statuscode as it's not needed)
    • Caveat of getStaticProps will be that it needs to generate routes at build time instead of at runtime, if fallback: true is used we can't do permanent redirects given the redirect will be client-side
    • I'd argue only a temporary redirect is the right way to go anyway for these cases as it's based on data and the data can change. So not being able to configure permanent even (which is what this PR does) however I'm not sure how other people think about this so I'd like to see it in the RFC
  • Only redirect should be returned given that Next.js should bail-out of continuing to render the page.

@justincy
Copy link
Contributor Author

@timneutkens You're right; it should've been an RFC first. Sorry about that. I think I'll have to let you write it. I'm not very familiar with getStaticProps and it's not clear to me why that also needs redirects.

@justincy justincy closed this May 28, 2020
@fbarbare
Copy link

fbarbare commented Jun 16, 2020

@justincy @timneutkens
Not sure where the RFC proposal is, but I would extend this a bit more and not only focus on redirects. It would be nice to be able to send status codes back to the browser based on user permissions also (e.g. 401, 403, ...).

I would say we could have more of an HTTP looking object like so:

{
  props: {  },
  status: number,
  headers: {  }
}

And Next could just check for the status and the headers.Location to know if it has to redirect or not.

@timneutkens
Copy link
Member

@justincy @timneutkens
Not sure where the RFC proposal is, but I would extend this a bit more and not only focus on redirects. It would be nice to be able to send status codes back to the browser based on user permissions also (e.g. 401, 403, ...).

I would say we could have more of an HTTP looking object like so:

{
  props: {  },
  status: number,
  headers: {  }
}

And Next could just check for the status and the headers.Location to know if it has to redirect or not.

This would only work for getServerSideProps so that would not be an ideal api. Features have to work for both SSG and SSR, otherwise people deoptimize their application where it isn't needed.

@fairbairn
Copy link

It would seem the solution would have to...

  • allow it to be easily defined in the SSR or SSG function (where the logic lives for the decision)
  • in the case of SSR, it should have an option (if the page has not yet been rendered), to respond with a 302 natively, no html is generated, just as if a bot had hit it (this also avoids the render, redirect flash we all want to avoid)
  • in the case of SSR, and it's updating just the .json data response, it would need to trigger client-side code to perform the redirect
  • in the case of SSG, it would perform a client-side code redirect, a bot would actually get a copy of the static page and would not redirect elsewhere unless it processed javascript - but this is probably fine.

You need redirection logic in both server side and client side code to handle (elegantly) all of the cases above with a single approach.

@timneutkens what do you think?

@timneutkens
Copy link
Member

I wrote the RFC for this: #14890

@fairbairn
Copy link

Ah thanks, yes I had already read that. Since 9.5 is now out, do you think it will get some attention? Just trying to assess timing as we might have to hack out an alternative in the meantime. Thanks Tim.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants