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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7672842
redirect existing root paths to the corresponding defaultLanguage path
pettinarip Jul 19, 2022
4b1d8f5
refactor
pettinarip Jul 19, 2022
a0c018b
disable generateMatchPathRewrites from plugins
pettinarip Jul 19, 2022
b7ba263
define 404 redirects with correct status code
pettinarip Jul 19, 2022
afe0925
only do client side redirect on dev
pettinarip Jul 20, 2022
439237e
only create redirects on productions or in cases where we have a server
pettinarip Jul 21, 2022
0c25a47
Merge branch 'fix-nested-redirects' into resolve-404-pages
pettinarip Jul 21, 2022
02267b3
update redirects json
pettinarip Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion gatsby-browser.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
defaultLanguage,
isLang,
} from "./src/utils/languages"
import { IS_DEV } from "./src/utils/env"
import { Context } from "./src/types"

// Default languages included:
Expand All @@ -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 /.

let detected =
window.localStorage.getItem("eth-org-language") ||
browserLang({
Expand Down
14 changes: 12 additions & 2 deletions gatsby-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,19 @@ const config: GatsbyConfig = {
},
},
// Needed for Gatsby Cloud redirect support
`gatsby-plugin-gatsby-cloud`,
{
resolve: `gatsby-plugin-gatsby-cloud`,
options: {
generateMatchPathRewrites: false,
},
},
// Creates `_redirects` & `_headers` build files for Netlify
`gatsby-plugin-netlify`,
{
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.

ref https://www.gatsbyjs.com/plugins/gatsby-plugin-gatsby-cloud/#configuration

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.

},
},
],
// https://www.gatsbyjs.com/docs/reference/release-notes/v2.28/#feature-flags-in-gatsby-configjs
flags: {
Expand Down
69 changes: 58 additions & 11 deletions gatsby-node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ import redirects from "./redirects.json"

const exec = util.promisify(child_process.exec)

const commonRedirectProps = {
isPermanent: true,
ignoreCase: true,
force: true,
}

/**
* Markdown isOutdated check
* Parse header ids in markdown file (both translated and english) and compare their info structure.
Expand Down Expand Up @@ -190,13 +196,11 @@ export const createPages: GatsbyNode<any, Context>["createPages"] = async ({
}) => {
const { createPage, createRedirect } = actions

// server side redirects
// custom redirects defined in `redirects.json`
redirects.forEach((redirect) => {
createRedirect({
...commonRedirectProps,
...redirect,
isPermanent: true,
ignoreCase: true,
force: true,
})
})

Expand Down Expand Up @@ -260,6 +264,12 @@ export const createPages: GatsbyNode<any, Context>["createPages"] = async ({
// e.g. English file: "src/content/community/index.md"
// e.g. corresponding German file: "src/content/translations/de/community/index.md"
if (language === defaultLanguage) {
createRedirect({
...commonRedirectProps,
fromPath: slug.slice(3),
toPath: slug,
})

for (const lang of supportedLanguages) {
const splitPath = relativePath.split("/")
splitPath.splice(2, 0, `translations/${lang}`)
Expand Down Expand Up @@ -315,6 +325,14 @@ export const createPages: GatsbyNode<any, Context>["createPages"] = async ({
// we can remove this logic and the `/pages-conditional/` directory.
const outdatedMarkdown = [`eth`, `dapps`, `wallets`, `what-is-ethereum`]
outdatedMarkdown.forEach((page) => {
const originalPath = `/${page}/`

createRedirect({
...commonRedirectProps,
fromPath: originalPath,
toPath: `/${defaultLanguage}${originalPath}`,
})

supportedLanguages.forEach(async (lang) => {
const markdownPath = path.resolve(
`src/content/translations/${lang}/${page}/index.md`
Expand All @@ -326,7 +344,7 @@ export const createPages: GatsbyNode<any, Context>["createPages"] = async ({
page,
lang
)
const originalPath = `/${page}/`

const slug = `/${lang}${originalPath}`

createPage<Context>({
Expand Down Expand Up @@ -356,14 +374,29 @@ export const onCreatePage: GatsbyNode<any, Context>["onCreatePage"] = async ({
page,
actions,
}) => {
const { createPage, deletePage } = actions
const { createPage, deletePage, createRedirect } = actions

// create routes without the lang prefix e.g. `/{path}` as our i18n plugin
// only creates `/{lang}/{path}` routes. This is useful on dev env to avoid
// getting a 404 since we don't have server side redirects
if (IS_DEV && page.path.startsWith(`/${defaultLanguage}`)) {
const isDefaultLang = page.path.startsWith(`/${defaultLanguage}`)

if (isDefaultLang) {
const path = page.path.slice(3)
createPage({ ...page, path })

if (IS_DEV) {
// create routes without the lang prefix e.g. `/{path}` as our i18n plugin
// only creates `/{lang}/{path}` routes. This is useful on dev env to avoid
// getting a 404 since we don't have server side redirects
createPage({ ...page, path })
}

if (!IS_DEV && !path.match(/^\/404(\/|.html)$/)) {
// on prod, indicate our servers to redirect the root paths to the
// `/{defaultLang}/{path}`
createRedirect({
...commonRedirectProps,
fromPath: path,
toPath: page.path,
})
}
}

const isTranslated = page.context.locale !== defaultLanguage
Expand All @@ -387,6 +420,20 @@ export const onCreatePage: GatsbyNode<any, Context>["onCreatePage"] = async ({
}
}

export const onPostBootstrap: GatsbyNode["onPostBootstrap"] = ({ actions }) => {
const { createRedirect } = actions

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

@pettinarip pettinarip Jul 20, 2022 •
edited
Loading

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

createRedirect({
...commonRedirectProps,
fromPath: `/${lang}/*`,
toPath: `/${lang}/404`,
statusCode: 404,
force: false,
})
})
}

export const createSchemaCustomization: GatsbyNode["createSchemaCustomization"] =
({ actions, schema }) => {
const { createTypes } = actions
Expand Down
92 changes: 0 additions & 92 deletions redirects.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,94 +47,14 @@
"fromPath": "/beginners",
"toPath": "/en/what-is-ethereum/"
},
{
"fromPath": "/",
"toPath": "/en/"
},
{
"fromPath": "/what-is-ethereum/",
"toPath": "/en/what-is-ethereum/"
},
{
"fromPath": "/eth/",
"toPath": "/en/eth/"
},
{
"fromPath": "/dapps/",
"toPath": "/en/dapps/"
},
{
"fromPath": "/wallets/",
"toPath": "/en/wallets/"
},
{
"fromPath": "/eth2/",
"toPath": "/en/upgrades/"
},
{
"fromPath": "/upgrades/",
"toPath": "/en/upgrades/"
},
{
"fromPath": "/staking/",
"toPath": "/en/staking/"
},
{
"fromPath": "/learn/",
"toPath": "/en/learn/"
},
{
"fromPath": "/community/",
"toPath": "/en/community/"
},
{
"fromPath": "/build/",
"toPath": "/en/developers/learning-tools/"
},
{
"fromPath": "/developers/",
"toPath": "/en/developers/"
},
{
"fromPath": "/enterprise/",
"toPath": "/en/enterprise/"
},
{
"fromPath": "/whitepaper/",
"toPath": "/en/whitepaper/"
},
{
"fromPath": "/foundation/",
"toPath": "/en/foundation/"
},
{
"fromPath": "/eips/",
"toPath": "/en/eips/"
},
{
"fromPath": "/about/",
"toPath": "/en/about/"
},
{
"fromPath": "/privacy-policy/",
"toPath": "/en/privacy-policy/"
},
{
"fromPath": "/terms-of-use/",
"toPath": "/en/terms-of-use/"
},
{
"fromPath": "/cookie-policy/",
"toPath": "/en/cookie-policy/"
},
{
"fromPath": "/languages/",
"toPath": "/en/languages/"
},
{
"fromPath": "/enterprise/",
"toPath": "/en/enterprise/"
},
{
"fromPath": "/java/",
"toPath": "/en/developers/docs/programming-languages/java/"
Expand Down Expand Up @@ -167,26 +87,14 @@
"fromPath": "/dart/",
"toPath": "/en/developers/docs/programming-languages/dart/"
},
{
"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..

"toPath": "/en/nft/"
},
{
"fromPath": "/dao/",
"toPath": "/en/dao/"
},
{
"fromPath": "/daos/",
"toPath": "/en/dao/"
},
{
"fromPath": "/defi/",
"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.

"toPath": "/en/layer-2/"
Expand Down