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 404 URLs on path-based i18n projects #1732

Merged
merged 4 commits into from Feb 8, 2024
Merged

Fix 404 URLs on path-based i18n projects #1732

merged 4 commits into from Feb 8, 2024

Conversation

blittle
Copy link
Contributor

@blittle blittle commented Feb 6, 2024

WHY are these changes introduced?

Hydrogen offers merchants the ability to easily set up localized paths for new projects. This means the active country and language code are in a path prefix. For example, a product page exists at /fr-ca/products/hydrogen. The locale prefix is optional, so /products/hydrogen also exists, where a default locale is used. The way to configure this in Remix is with optional route segments. Hydrogen generates a route file named: ($locale).products.$productHandle.tsx.

This works well until the user goes to an unknown route. In this case, the app needs to return a 404 response. Each of the following routes need to generate a 404:

  1. https://hydrogen.shop/doesNotExist
  2. https://hydrogen.shop/doesNotExist/products

The problem is that, even though neither route exists, both routes still match for Remix. This is because ($locale) is an optional segment, and the index route and products route both match and render at each route respectively.

The fix is to add a pathless route ($locale).tsx that includes logic to throw a 404.

WHAT is this pull request doing?

  1. Update the demo store and starter template to include a ($locale).tsx pathless route.
  2. Add logic to the CLI to make sure it gets generated on new projects.

HOW to test your changes?

Test the following routes:

  1. 404 - /doesNotExist
  2. 404 - /doesNotExist/collections
  3. 404 - /en-ca/doesNotExist
  4. 200 - /collections
  5. 200 - /en-ca/collections

Test each of these routes on both the demo store and a newly generated starter template. Do not test inside the skeleton template directly (because it doesn't have i18n configured). Instead, generate a new project with h2 init at the root of the repo.

Also verify:

  1. ($locale).jsx is generated for JS projects
  2. ($locale).tsx is generated for TS projects
  3. No ($locale).tsx is generated for non-path-based i18n projects

Post-merge steps

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

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.

I've updated it a bit to work with adapter option and h2 generate route command 👍

@blittle blittle merged commit fcecfb2 into main Feb 8, 2024
10 checks passed
@blittle blittle deleted the bl-fix-locale-check branch February 8, 2024 13:22
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.

None yet

2 participants