Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

Redirect i18n URLs that would be valid without the locale prefix #2412

Merged
merged 5 commits into from
Apr 4, 2022

Conversation

jeffposnick
Copy link
Contributor

R: @devnook
CC: @agektmr
Fixes #2398

This shares a similar goal to GoogleChrome/web.dev#7283, but that PR was never merged, and the d.c.c. serving infrastructure differs from web.dev, so I wanted to write something fresh and focused.

The behavior implemented in this PR is:

  • If any navigation would return a 404 today, check whether replacing the first item in the path with the default locale (en) would lead to a valid HTML document.
    • If so, instead of returning a 404, return a 302 redirecting to the non-localized version of that URL.
    • If not, continue returning a 404 as before.

One approach that I considered, but ultimately decided against, was returning the HTML content from the default locale as a 200 response directly from the localized URL, instead of redirecting. That seemed riskier from the point of view of indexing, so I went with a redirect instead.

@jeffposnick jeffposnick requested a review from devnook March 24, 2022 15:57
@jeffposnick
Copy link
Contributor Author

I realize that we have some tests for server functionality, so I'll add a new test to this PR to cover the getNonLocalizedURL() logic.

@netlify
Copy link

netlify bot commented Mar 24, 2022

Deploy Preview for developer-chrome-com ready!

Name Link
🔨 Latest commit fa40b17
🔍 Latest deploy log https://app.netlify.com/sites/developer-chrome-com/deploys/624b1f54f69b6f00096771f2
😎 Deploy Preview https://deploy-preview-2412--developer-chrome-com.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@jeffposnick
Copy link
Contributor Author

I've added a few tests for the getNonLocalizedURL() logic to this PR.

@jeffposnick
Copy link
Contributor Author

Let's merge on Monday, as I would prefer there not to be any surprises with our server code over the weekend 😄

@devnook
Copy link
Contributor

devnook commented Apr 1, 2022

I noticed lhci is unhappy, because robots-txt changed location. Let's resolve it before merging.

@jeffposnick
Copy link
Contributor Author

Are we sure that LHCI error is legitimate? I don't see a https://developer.chrome.com/robots.txt on the production website, either. Let's chat about it on Monday.

Copy link
Contributor

@devnook devnook left a comment

Choose a reason for hiding this comment

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

With the new server logic we return a "404" page for /robots.txt, which is a "malformed" robots file, while previously it was a real native 404.

Simple solution would be to add a robots.txt same as in web.dev

@jeffposnick jeffposnick merged commit 12d44ca into main Apr 4, 2022
@jeffposnick jeffposnick deleted the redirect-i18n-404s branch April 4, 2022 18:14
Copy link

@Naveed-Iftikhar Naveed-Iftikhar left a comment

Choose a reason for hiding this comment

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

404 and 403 open

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.

[High priority] Non-translated articles return 404
3 participants