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

Resolve 404 pages #7137

Merged
merged 8 commits into from
Jul 21, 2022
Merged

Resolve 404 pages #7137

merged 8 commits into from
Jul 21, 2022

Conversation

pettinarip
Copy link
Member

@pettinarip pettinarip commented Jul 19, 2022

IMPORTANT: this PR was branched from #7135. Merge that PR first.


Testing site: https://pettinarip-eth-org.netlify.app/

We are implementing the Netlify docs: https://docs.netlify.com/routing/redirects/redirect-options/#custom-404-page-handling

Fix #5682
Also cover #6356

@gatsby-cloud
Copy link

gatsby-cloud bot commented Jul 19, 2022

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 34m

@pettinarip pettinarip force-pushed the resolve-404-pages branch 3 times, most recently from 62def04 to 8f252bc Compare July 20, 2022 14:15
@@ -42,7 +43,7 @@ export const wrapPageElement: GatsbyBrowser<

// client side redirect on paths that don't have a locale in them. Most useful
// on dev env where we don't have server redirects
if (!isLang(pathLocale)) {
if (IS_DEV && !isLang(pathLocale)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

On prod we have server side redirects. We don't need this. In dev is handy to avoid receiving a 404 page at /.

@pettinarip pettinarip marked this pull request as ready for review July 20, 2022 15:04
export const onPostBootstrap: GatsbyNode["onPostBootstrap"] = ({ actions }) => {
const { createRedirect } = actions

supportedLanguages.forEach((lang) => {
Copy link
Member Author

@pettinarip pettinarip Jul 20, 2022

Choose a reason for hiding this comment

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

Creating these 404 redirects once all the pages and other redirects are created. We need them at the end of the _redirects file.

Copy link
Member

Choose a reason for hiding this comment

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

So if understand, the _redirects file that is created when building contains all redirects for the pages on the site. The site tries to resolve one of those initial redirects for the url, but if it can't find it then falls back to these 404 redirects being created here? Hopefully, this question makes sense. If it doesn't let me know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question.

the _redirects file that is created when building contains all redirects for the pages on the site

Yes. Including the 404s. These files (_redirects & redirects.json) are only created for servers. They don't play any role in the local env.

The site tries to resolve one of those initial redirects for the url, but if it can't find it then falls back to these 404 redirects being created here?

Yes, I think the complete order is:

  1. url/path match file => display the file
  2. url/path match redirect => display the redirected file
  3. url/path doesn't match anything => display default 404 (in English)
    • unless it matches the 404 fall backs for each locale (the ones created here), e.g. /es/something, will display the /es/404 page

{
resolve: `gatsby-plugin-netlify`,
options: {
generateMatchPathRewrites: false,
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit more context on this. Our i18n plugin is generating these matchPath routes for each locale, for client side routing purposes:

if (newPage.path.match(/^\/[a-z]{2}\/404\/$/)) {
  // Match all paths starting with this code (apart from other valid paths)
  newPage.matchPath = `/${locale.code}/*`
}

If this flag is on, it would write in the _redirects file, something like:

/en/*  /en/404  200
/es/*  /es/404  200

Note the 200 status code. We don't want that. That will create a redirect. We want a 404 status code in there. That is why we are disabling this and creating those rules ourselves.

@@ -171,30 +95,10 @@
"fromPath": "/nft/",
"toPath": "/en/nft/"
},
{
"fromPath": "/nfts/",
Copy link
Member

Choose a reason for hiding this comment

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

Would we want to keep this redirect? I imagine the nft one above is more likely what was supposed to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gooood catch. Yes! exactly that, thanks. I've removed the wrong one. Fixing..

redirects.json Outdated
{
"fromPath": "/languages/",
"toPath": "/en/languages/"
},
{
"fromPath": "/enterprise/",
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the redirects you removed, is this one still necessary?

Copy link
Member

@corwintines corwintines Jul 20, 2022

Choose a reason for hiding this comment

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

Or is this one kept because there was 2 of the same redirects in this file 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch again! yes, any page that is created by us shouldn't have its declaration here. It is created automatically.

Copy link
Member

@corwintines corwintines left a comment

Choose a reason for hiding this comment

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

Nice! Glad you got this working!

Tested in both english and spanish.

Just had one question to double check I understand how this works: #7137 (comment)

"toPath": "/en/defi/"
},
{
"fromPath": "/layer2/",
Copy link
Member Author

Choose a reason for hiding this comment

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

This one should also exists. Will revert these last two redirects.

@pettinarip pettinarip merged commit 093fc69 into dev Jul 21, 2022
@pettinarip pettinarip deleted the resolve-404-pages branch July 21, 2022 16:26
@corwintines corwintines mentioned this pull request Jul 25, 2022
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.

Incorrect response codes
2 participants