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

Fix RSC Redirects #965

Merged
merged 4 commits into from
Mar 22, 2022
Merged

Fix RSC Redirects #965

merged 4 commits into from
Mar 22, 2022

Conversation

blittle
Copy link
Contributor

@blittle blittle commented Mar 21, 2022

Description

Fix RSC Redirects. Resolves #947

Additional context

Fix server redirects to work properly with RSC responses. For example, the redirect component within the starter template changes:

export default function Redirect({response}) {
-  response.redirect('/products/snowboard');
-  return <div>This page is redirected</div>;
+  return response.redirect('/products/snowboard');
}

This server component is executed two ways:

  1. When an app directly loads the redirect route, the server will render a 300 redirect with the proper location header.
  2. The app is already loaded, but the user navigates to the redirected route. We cannot 300 respond in this scenario, instead response.redirect(...) returns a component which will redirect on the client.

The redirect e2e test has been modified to cover this scenario.

@blittle blittle force-pushed the fix-redirect branch 4 times, most recently from 05fa230 to 2e05650 Compare March 21, 2022 17:55
@blittle blittle requested a review from a team March 21, 2022 17:55
Copy link
Contributor

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. This is way more elegant than I assumed it would be.

@blittle
Copy link
Contributor Author

blittle commented Mar 21, 2022

@mcvinci FYI there's some documentation changes in here

Copy link
Contributor

@mcvinci mcvinci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs changes look good! Thanks, @blittle!

Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Bret! Very simple and elegant solution.

I've noticed there are two Redirect.client.xyz.js files emitted when building the client, and it seems to be related to including Redirect component in ServerComponentResponse.server.js. I've no idea why because that file should not affect the client at all...
I think it is not a big issue anyway so we can merge this. Perhaps things will be clearer once we extract client stuff into hydrogen-ui.

@blittle blittle merged commit cdad13e into v1.x-2022-07 Mar 22, 2022
@blittle blittle deleted the fix-redirect branch March 22, 2022 14:29

useEffect(() => {
if (to.startsWith('http')) {
window.location.href = to;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be window.location.replace instead so its truly a redirect?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] RSC does not handle redirect
5 participants