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

Change intl gatsby plugin #7022

Merged
merged 17 commits into from Jul 14, 2022
Merged

Change intl gatsby plugin #7022

merged 17 commits into from Jul 14, 2022

Conversation

pettinarip
Copy link
Member

@pettinarip pettinarip commented Jul 12, 2022

Description

Replace gatsby-plugin-intl with gatsby-theme-i18n to fix memory issues.

Test:
https://ethereumorgwebsitedev01-gatsbythemei18n.gtsb.io/

Context:

TODO:

  • fix localized links
  • fix banner's logic
  • check redirects

@github-actions github-actions bot added content 🖋️ This involves copy additions or edits dependencies 📦 Changes related to project dependencies review needed 👀 labels Jul 12, 2022
@gatsby-cloud
Copy link

gatsby-cloud bot commented Jul 12, 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: 27m

@pettinarip pettinarip self-assigned this Jul 12, 2022
const queryParams = search || ""
const newUrl = withPrefix(`/${detected}${originalPath}${queryParams}`)
window.localStorage.setItem("eth-org-language", detected)
window.location.replace(newUrl)
Copy link
Member Author

Choose a reason for hiding this comment

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

As the comment says, this is only used in dev envs. This was extracted from what the previous plugin was doing https://github.com/wiziple/gatsby-plugin-intl/blob/master/src/wrap-page.js#L74

The new plugin doesn't do this for us.

prefixDefault: true,
locales: supportedLanguages.length
? supportedLanguages.join(" ")
: null,
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't pass an empty array here []. We must pass null if we want to build all the langs.

path: langSlug,
component: path.resolve(`src/templates/${template}.tsx`),
context: {
language: lang,
Copy link
Member Author

Choose a reason for hiding this comment

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

Keeping most of these context values for backward compatibility. I'll probably refactor this in next iterations.

language: lang,
defaultLanguage,
languages: supportedLanguages,
messages: getMessages("./src/intl/", lang),
Copy link
Member Author

Choose a reason for hiding this comment

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

THE only reason why we are changing the plugin. This is causing memory issues.

"name": "English",
"localName": "English",
"langDir": "ltr",
"dateFormat": "MM/DD/YYYY"
Copy link
Member Author

Choose a reason for hiding this comment

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

In next iterations, we need to start using these values.

@@ -62,6 +64,7 @@
"react-helmet": "^6.1.0",
"react-icons": "^4.3.1",
"react-instantsearch-dom": "^6.6.0",
"react-intl": "^3.12.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Current version is v6. We keep using v3 to avoid changing too many things.

@@ -222,4 +224,17 @@ const Link: React.FC<IProps> = ({
)
}

export function navigate(
Copy link
Member Author

Choose a reason for hiding this comment

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

New plugin doesn't provide this function. Implemented a similar approach as the previous one was doing https://github.com/wiziple/gatsby-plugin-intl/blob/master/src/link.js#L42

@pettinarip pettinarip changed the title [WIP] Change intl gatsby plugin Change intl gatsby plugin Jul 14, 2022
@pettinarip pettinarip marked this pull request as ready for review July 14, 2022 17:59
Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

@pettinarip This is awesome! To the team, hopped on a call with Pablo to walk through this and test it out. Redirects seems to be working appropriately and all the languages are loading as expected.

Build this from scratch locally with 16GB RAM with all languages and it built in under 11 minutes, and node never used over 2-3 GB RAM at any given time. Just one test case, but this is orders of magnitude faster than before.

Given our master branch deploy build is currently failing as a result of immense RAM demands, going to bring this in and get a patch deploy out immediately following.

SOLID WORK!! This is going to help tremendously 🎉 💪

@wackerow
Copy link
Member

Edit on above: master build is not currently failing fortunately, since the re-triggered build ended up passing. This buys some time from rushing a deploy to master, but still going to bring this into dev so we can start taking advantage of its efficiencies and test it out before it goes live.

@wackerow wackerow merged commit 58968e2 into dev Jul 14, 2022
@wackerow wackerow deleted the gatsby-theme-i18n branch July 14, 2022 20:11
@wackerow wackerow mentioned this pull request Jul 14, 2022

const [, pathLocale] = pathname.split("/")

// client side redirect on paths that don't have a locale in them. Most useful
Copy link
Contributor

Choose a reason for hiding this comment

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

@nhsz @wackerow should we take a similar approach on the blog?

const queryParams = search || ""
const newUrl = withPrefix(`/${detected}${originalPath}${queryParams}`)
window.localStorage.setItem("eth-org-language", detected)
window.location.replace(newUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it matters but should we use the Gatsby navigate helper here instead of the window API? https://www.gatsbyjs.com/docs/reference/built-in-components/gatsby-link/#how-to-use-the-navigate-helper-function

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea. I think we could do that, yes.

})

if (!isLang(detected)) {
detected = defaultLanguage
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we always fallback to defaultLanguage? Better option might be to inspect the user's browser preference (that's what gatsby-plugin-intl did).

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is what gatsby-plugin-intl uses:
https://github.com/wiziple/browser-lang/blob/master/src/index.js

We could (probably?) do something simpler, e.g. by just using the Navigator.language API:
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/language

See: https://phrase.com/blog/posts/detecting-browser-language-preference-with-javascript/

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we already added browser-lang as a dependency in package.json anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, a few lines above this (line 48) we are using browser-lang. This was taken from gatsby-plugin-intl.

"name": "Arabic",
"localName": "العربية",
"langDir": "rtl",
"dateFormat": "MM/DD/YYYY"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't date formatting vary by language?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need to rely on these? We have the getLocaleTimestamp helper for displaying date formats.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I've added these fields because they were required by the gatsby-theme-i18n plugin. For now, we are not using langDir and dateFormat from here. Those things are still working as before.

  • I agree we should not use dateFormat from this file. We should keep using what we currently have.
  • However, I think we could consume langDir from this config file instead of what we are currently doing (function isLangRightToLeft).

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha - I'm on board.

"hrefLang": "en",
"name": "English",
"localName": "English",
"langDir": "ltr",
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't appear that we actually use langDir for anything - should we?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know we use the isLangRightToLeft helper for that currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. Related to prev comment. I haven't done that yet because I didn't want to add too many changes but I think that for langDir we should use it from here as the source of truth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits dependencies 📦 Changes related to project dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants