Skip to content

Move redirect logic to the entry file#362

Merged
frandiox merged 8 commits intov0.x-2022-10from
fd-move-redirects
Jan 12, 2023
Merged

Move redirect logic to the entry file#362
frandiox merged 8 commits intov0.x-2022-10from
fd-move-redirects

Conversation

@frandiox
Copy link
Copy Markdown
Contributor

@frandiox frandiox commented Jan 11, 2023

Instead of calling notFoundMaybeRedirect, this changes it to throw 404 responses in the routes and then redirect conditionally in server.ts.

Some things to consider:

  • I've renamed the function to storefrontRedirect. Open to other names 🙏
  • We check the SFAPI for every 404.
  • We need to relay the existing 404 response when not finding redirects to reuse the NotFound template that Remix has already rendered (otherwise we create an empty 404 response without HTML).
  • Even if there is a network error while fetching the redirects, we don't throw and instead just return the existing 404 response.

Questions:

  • Should we move the redirect logic to the app and remove it from Hydrogen? Or we keep it in the package?

🎩 :

  • /collections/hydrogen should redirect to /collections/backcountry.
  • /collections/xyz should show a 404 page.
  • /collections/xyz?return_to=/about and /collections/xyz?redirect=/about should redirect accordingly.
  • /collections/xyz?return_to=https%3A%2F%2Fgoogle.com should show a 404 page and warn in the terminal about cross-domain redirects.

@frandiox frandiox requested review from a team and blittle January 11, 2023 19:01
@github-actions

This comment has been minimized.

@frandiox frandiox mentioned this pull request Jan 11, 2023
Comment thread packages/hydrogen/src/routing/redirect.ts Outdated
Copy link
Copy Markdown
Contributor

@frehner frehner left a comment

Choose a reason for hiding this comment

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

Maybe things have changed, but the last time I talked with @blittle the idea was to remove all the redirect stuff out of each route file and only put it into server.ts?

[edit] whoops I think I misread something, that's what it appears you're doing. Excellent. 👍

return false;
}

type RedirectQueryType = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general we should probably never make our own Storefront API types - you can get this type by doing:

import type {UrlRedirectConnection} from '@shopify/hydrogen-react/storefront-api-types';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did that at the beginning and then reverted it to the original hardcoded version because I thought @shopify/hydrogen didn't depend on @shopify/hydrogen-react. Turns out it has it as a peer dependency so I will change it again 😅

Comment thread templates/demo-store/server.ts
Comment thread packages/hydrogen/src/routing/redirect.ts Outdated
Copy link
Copy Markdown
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

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

Looks good after that query issue is fixed. Thx Fran!

frandiox and others added 2 commits January 12, 2023 10:49
@frandiox frandiox merged commit 1766696 into v0.x-2022-10 Jan 12, 2023
@frandiox frandiox deleted the fd-move-redirects branch January 12, 2023 10:15
@github-actions github-actions Bot mentioned this pull request Feb 8, 2023
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.

3 participants