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 #89

Closed
jplhomer opened this issue Jun 7, 2021 · 11 comments
Closed

Support redirects #89

jplhomer opened this issue Jun 7, 2021 · 11 comments
Labels
framework Related to framework aspects of Hydrogen
Milestone

Comments

@jplhomer
Copy link
Contributor

jplhomer commented Jun 7, 2021

Just need to make sure this works on server and client.

@igrigorik
Copy link

@jplhomer what's our thinking on how redirects are set up by the developer?

@jplhomer
Copy link
Contributor Author

@igrigorik I'm kicking around a few options:

Option 1. <Redirect>

This is pulled directly from react-router: https://reactrouter.com/web/api/Redirect.

Note that this requires us to inspect the context object in the provider after the page has rendered, meaning the user will have to call response.doNotStream() in order for us to set the response headers & status code accordingly.

The nice thing about this option is that it is modeled after an existing popular API. IMO people will expect this to work out of the box, since we're using the same library throughout.

Option 2. response.redirect(url: string)

Add a custom redirect() function to our ServerComponentResponse class. Pretty straightforward usage here.

Option 3. Do it by hand

There's nothing stopping the developer from setting response.headers.Location and response.status on their own.


Maybe we support all three? Any thoughts?

@igrigorik
Copy link

  1. I suspect you're right on (1), we need it but it's the slowest path.
  2. We should probably expose the "standard" res.redirect([status,] path)...?
  3. Assuming we give access to manipulate headers, nothing to do here, right?

Is there a declarative path that Oxygen could pick up directly? E.g. next allows you to enumerate an array of redirect rules, which Oxygen could consume directly and build out a routing map.. or we could take those roles and push them into CF workers for faster routing, etc.

@jplhomer
Copy link
Contributor Author

Update: react-router is dropping <Redirect> in v6. Makes sense: https://gist.github.com/mjackson/b5748add2795ce7448a366ae8f8ae3bb

I think we lean into our res.redirect path + potential static analysis or array of redirect rules.

@frandiox
Copy link
Contributor

Question: How would it behave in SPA takeover? Should the developer take care of that and just rely on react-router to do a redirect?

Or maybe this is a no-issue, I don't know yet how this plays with RSC 🤔

@jplhomer
Copy link
Contributor Author

@frandiox I just wondered the same... but I think we're good:

  1. User clicks SPA link, which setServerState('page', '/a')
  2. This triggers AJAX call to /react?state={page:a}
  3. This resolves to App.server which either loads the page component or deals with custom redirect logic the user has built in
  4. A status: 301; location: /b is set on the response to the AJAX call
  5. fetch on the client automatically calls the new endpoint
  6. The new endpoint resolves the redirect, which then shows the content of the redirected page ✨

The only outstanding questions I have:

  1. The URL will likely be incorrect: the 301 in the fetch call will not history.pushState. We'll need to add a mechanism that checks for a 301 and do so ourselves likely.
  2. Will our response.read() in Cache.client error out if it sees and empty body with a 301? Or does fetch transparently handle that behind the scenes?

@frandiox
Copy link
Contributor

frandiox commented Oct 25, 2021

  1. The URL will likely be incorrect: the 301 in the fetch call will not history.pushState. We'll need to add a mechanism that checks for a 301 and do so ourselves likely.

I think this is the main problem I thought of. And since 3xx responses are opaque to fetch, we can't even know that there was a redirect (we would just receive the page B props/components when we asked for page A).

Perhaps we could include the requested page parameter in a response header? Say we request pageA but the server redirects to pageB and browser fetch makes that request internally automatically. When pageB is handled, we return x-requested-page: pageB. Then, when our fetch is resolved in the browser, we can see that the returned header is pageB but we in fact requested pageA ==> historyPush 🤔

-- edit: or perhaps the /react request already contains the "page" in its body and we can just check that?

  1. Will our response.read() in Cache.client error out if it sees and empty body with a 301? Or does fetch transparently handle that behind the scenes?

As far as I understand, fetch handles redirects internally unless you provide { redirect: 'manual' } option, in which case you get an opaque response without any interesting information.
I'm not sure how this all plays with the cache, though.

@jplhomer jplhomer transferred this issue from another repository Nov 5, 2021
@frandiox frandiox mentioned this issue Nov 9, 2021
7 tasks
@frandiox
Copy link
Contributor

frandiox commented Nov 17, 2021

Thoughts on https://github.com/Shopify/hydrogen/pull/172 (using option 2)?

  1. User clicks SPA link, which setServerState('page', '/a')
  2. This triggers AJAX call to /react?state={page:a}
  3. This resolves to App.server which either loads the page component or deals with custom redirect logic the user has built in
  4. A status: 301; location: /b is set on the response to the AJAX call
  5. fetch on the client automatically calls the new endpoint
  6. The new endpoint resolves the redirect, which then shows the content of the redirected page ✨

The problem is that fetch would get an HTML response from /b redirect instead of the expected "wire" syntax. There are workarounds, though, it's mentioned in the linked PR.

@morganmccunn morganmccunn added this to the v1.0.0 milestone Nov 17, 2021
@jplhomer jplhomer added the framework Related to framework aspects of Hydrogen label Nov 22, 2021
@davecyen
Copy link
Contributor

davecyen commented Jan 4, 2022

The Admin API currently provides a Redirect object, which exposes the redirect paths that the merchant has set up in their shop's admin settings:
https://shopify.dev/api/admin-rest/2021-10/resources/redirect#top

Some merchants may have dozens of URL redirects already set up for their liquid shop. Then, when migrating to Hydrogen, it would be a pain to have to manually recreate these all.

Some initial questions:

  • Will Hydrogen's Redirect talk to the Admin API's Redirect?
  • If so, what will be the default behavior be when a merchant launches a hydrogen storefront? Will their Admin redirects automatically be duplicated in Hydrogen?
  • Are new redirects that are created in Hydrogen readable and editable from Admin? And vice versa?
  • How would this work with multiple storefronts?

For reference, screenshot below of the URL Redirects settings page in admin:
Screen Shot 2022-01-04 at 3 35 06 PM

@jplhomer
Copy link
Contributor Author

jplhomer commented Jan 5, 2022

Will Hydrogen's Redirect talk to the Admin API's Redirect?

No, since Hydrogen talks to the Storefront API and not the Admin API. If the Storefront API exposes redirect data (similar to what we're exploring for nav/menu data), then it would be a possibility.

Since Hydrogen is developer-focused, devs should be able to enter and program their own redirects.

We could provide some utilities in e.g. shopify.config.js to make this easier, similar to what Next.js does. But for now, I think bespoke is our best bet.

@jplhomer
Copy link
Contributor Author

Closing in favor of the new routing API and client router we've built https://github.com/Shopify/hydrogen/discussions/569

rafaelstz pushed a commit to rafaelstz/hydrogen that referenced this issue Mar 4, 2023
…w-27ha

Add Oxygen deployment workflow file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework Related to framework aspects of Hydrogen
Projects
None yet
Development

No branches or pull requests

5 participants