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

feature(www): Translate text through js-lingui #21633

Merged
merged 28 commits into from Mar 13, 2020
Merged

feature(www): Translate text through js-lingui #21633

merged 28 commits into from Mar 13, 2020

Conversation

@tesseralis
Copy link
Collaborator

tesseralis commented Feb 21, 2020

Description

Translate text through Lingui.

TODO:

  • Run lingui extract as a pre-commit hook (I don't know how to do this)
  • Performance test
  • Lazy loading
  • Get input from translators on format
    • posted on discord
  • Figure out why i18n isn't working
  • Maybe switch to Lingui v3?

Follow-ups:

  • Instructions for translators
  • Translate more text
  • Do a check on build to make sure no new strings need to be translated

Build

https://build-e8e3cbac-6776-4a0d-890c-543cfd6d0c2b.gtsb.io/docs/

(note: since languages aren't configured, it doesn't have any other languages than english)

Screenshot

screenshot of gatsbyjs.org in japanese showing translated 'previous' and 'next' links

Related Issues

Fixes: #19102
Alternate of: #21024

@tesseralis tesseralis requested a review from gatsbyjs/website as a code owner Feb 21, 2020
@tesseralis tesseralis requested review from pieh and LekoArts Feb 21, 2020
tesseralis added 4 commits Feb 21, 2020
@@ -111,7 +113,7 @@
"main": "n/a",
"private": true,
"scripts": {
"build": "gatsby build",
"build": "lingui compile && gatsby build",

This comment has been minimized.

Copy link
@tesseralis

tesseralis Feb 21, 2020

Author Collaborator

Is this the right place to put this? Can we do prebuild or something?

This comment has been minimized.

Copy link
@LekoArts

LekoArts Feb 21, 2020

Contributor

You could install husky and lint-staged to run a pre-commit hook:

gatsby/package.json

Lines 68 to 86 in 3894840

"lint-staged": {
"*.{js,jsx}": [
"eslint --cache --ext .js,.jsx,.ts,.tsx --fix",
"git add"
],
"*.{md,css,scss,yaml,yml}": [
"prettier --write",
"git add"
],
"*.svg": [
"svgo --pretty --indent=2 --config=svgo.yml --multipass",
"git add"
]
},
"husky": {
"hooks": {
"pre-commit": "lint-staged || node scripts/on-lint-error.js"
}
},

Or does this only need to be run before a build and not necessarily a commit?

This comment has been minimized.

Copy link
@tesseralis

tesseralis Feb 21, 2020

Author Collaborator

lingui extract is the thing that generates the .po files -- those need to be in the codebase so that translators know what they need to translate, so it needs to be pre-commit.

Should I install husky on www or is there some way to use the one configured on top level?

This comment has been minimized.

Copy link
@LekoArts

LekoArts Feb 21, 2020

Contributor

www is separate so to speak, so yeah, go ahead with installing it there 👍

This comment has been minimized.

Copy link
@tesseralis

tesseralis Feb 21, 2020

Author Collaborator

I added husky and lint-staged but ran into two issues:

  • I can’t figure out how to tell lint-stage to commit the generated po files.
  • lingui extract takes a few seconds to run, and doesn’t have any options to only run for specific files.

I don’t think its good to run it on every commit, especially since most www changes will not be string related. I was thinking that on build we could watch the translation files and fail build if there are any changes?

@tesseralis tesseralis requested a review from gatsbyjs/core as a code owner Feb 21, 2020
@tesseralis

This comment has been minimized.

Copy link
Collaborator Author

tesseralis commented Feb 21, 2020

I decided to look up Lingui at @LekoArts 's recommendation and so far I really like it!

Pros:

  • The CLI is really useful and can be easily integrated into our workflow. Running lingui extract means we can generate translation strings out of text without worrying about them being made obsolete.
  • It's very cool that it uses macros for the majority of its work, meaning minimal bundle size when built.
  • It supports the PO translation format, which I really like so far. It's more verbose, but it adds more metadata: including file names and line numbers means translators can easily look up the context of a line.

Cons:

  • No useLingui hook, although that's coming in the next major release.
tesseralis added 10 commits Feb 21, 2020
@tesseralis

This comment has been minimized.

Copy link
Collaborator Author

tesseralis commented Feb 22, 2020

Perf results:

Master: https://www.webpagetest.org/result/200222_AD_38996de9f909f3d35dd7b0e6a223a135/1/details/

Lingui: https://www.webpagetest.org/result/200222_AD_38996de9f909f3d35dd7b0e6a223a135/1/details/

Lingui adds about 11kB, or 2% of our bundle. The thing that's surprising is that the compiled text is tiny:

lingui-locale-data

en/messages.js, which has translated versions of each text, is 1.37kB, or 0.2%.

This might change once we start trying to translate more text specific to certain pages, but we can always lazy-load languages if it becomes an issue. Unfortunately it doesn't seem like Lingui supports splitting up a language into multiple catalogs just now, but maybe that's an improvement that the next version can have?

For now, I can try to do a stress test and try to put as much content as I can under Trans components to see what it's like. But I think we're good :)

@tesseralis

This comment has been minimized.

Copy link
Collaborator Author

tesseralis commented Feb 24, 2020

I set up a branch to see what a fully translated bundle size would look like, taking some of our most text-heavy sections and converting them to p0 files.

The increase in bundle is something like this:

source map explorer screenshot showing 56kb of data per language

Obviously... 56kb per language is not good. But I don't think we'll ever actually get there. Most of this content can be moved to Markdown so that they can be sourced in the GraphQL. We should only really use Lingui for small bits of UI -- anything a paragraph or more can be made a markdown fragment.

Unfortunately, lingui doesn't let you split up message files, though this is in development. Another workaround is to split up the site into themes and create a different message file for each section.

# Conflicts:
#	www/i18n.json
@AishaBlake

This comment has been minimized.

Copy link
Contributor

AishaBlake commented Feb 26, 2020

So Lingui still seems like our best option at this point. I'm curious what Core folks have to say about the performance implications of this solution but I think it's the best workflow on the table.

@tesseralis

This comment has been minimized.

Copy link
Collaborator Author

tesseralis commented Feb 27, 2020

Well. @LekoArts was the one who originally suggested it because it has a smaller bundle size :)

@@ -110,6 +111,24 @@
"license": "MIT",
"main": "n/a",
"private": true,
"babel": {
"presets": ["babel-preset-gatsby"]

This comment has been minimized.

Copy link
@tesseralis

tesseralis Feb 27, 2020

Author Collaborator

@wardpeet, @pvdz told me that you might know what's up. For some reason, Lingui requires this configuration in order for the CLI to run properly and let us do lingui extract. But it shouldn't be necessary, since it's the default included in gatsby.

This comment has been minimized.

Copy link
@pvdz

pvdz Feb 27, 2020

Contributor

(This was in relation to webpack, but it turns out this was actually needed because lingui couldn't figure out our preset without a hint like this)

@pvdz

This comment has been minimized.

Copy link
Contributor

pvdz commented Feb 27, 2020

This looks great!

I would strongly suggest to pick a greppable needle for this. t is nice and short and impossible to search for (arguably, even t` is). Maybe gt or gtx would be better, at least then we can find all translatable strings through a grep. Maybe import {gt} from 'gatsby-translate' which calls t underwater.

I also see a few different ways of doing translation, like I saw i18n._(t`. While I'm sure they have reasons, I wonder whether those can't be merged or abstracted somehow? Can't they use the same thing, or at least, have a common (greppable) name?

(Giving it a unique greppable token would make a migration or translation-specific change in the future a lot easier)

@tesseralis

This comment has been minimized.

Copy link
Collaborator Author

tesseralis commented Feb 27, 2020

\Wt` seems to work well for me.

t is a fairly common convention among translation libs (for example, i18next, polyglot). The other problem is that it's a macro, so I'm not sure you can just wrap it in a function and have it work. (for example, the lingui extract command looks specifically for these messages and might not work if we get change them.

I was considering using a wrapper library when I was using react-intl, but I feel like it's overengineering and it's hard to anticipate what the mapping would be. I think it would be more beneficial to add docs to our contributing guide with what the different functions do (with links to Lingui's docs, of course). I don't think it's an issue.

@pvdz

This comment has been minimized.

Copy link
Contributor

pvdz commented Feb 27, 2020

That's a good point. I'm used to fbt, which was always a good needle to search for :)

And while we could work around the transform (it might be smart enough to detect a renamed export? or have an option?) you're right that it's probably just asking for trouble.

Ok, ignore me.

@AishaBlake

This comment has been minimized.

Copy link
Contributor

AishaBlake commented Feb 28, 2020

@tesseralis, feels like this is quite close. Leaving off lazy loading for now? If you're good to move forward, go ahead and take care of the conflicts and I'll have one more look.

@AishaBlake

This comment has been minimized.

Copy link
Contributor

AishaBlake commented Mar 5, 2020

I've been trying to run this again but it looks like it's having trouble finding the messages.po file for English. Maybe we could schedule some time looking at this together, @tesseralis? I'd like to be totally sure that I understand what's happening anyway.

tesseralis added 4 commits Mar 9, 2020
@AishaBlake

This comment has been minimized.

Copy link
Contributor

AishaBlake commented Mar 11, 2020

@tesseralis I've been looking through code and I think I understand this better. Also, it's building after that last round of changes! Should I be seeing these translations in the browser if I'm on /ja at this point? I do see the messages in /data/locales.

@tesseralis

This comment has been minimized.

Copy link
Collaborator Author

tesseralis commented Mar 11, 2020

@aisha, you shouldn't see them on the browser since not configured with any locales. You'd have to set it up locally and run it with LOCALES=ja in your .env.development. If you scroll down to the footer, those messages should be translated.

Copy link
Contributor

AishaBlake left a comment

YES! This looks awesome. Thanks for your patience while I waded in. I don't see anything that should hold up this PR. @LekoArts, any problems with me merging this? I'm looking to merge the docs (#21805) in at roughly the same time and am nearly finished reviewing that PR as well.

Copy link
Contributor

LekoArts left a comment

LGTM 👍 Merge whenever wanted :)

@AishaBlake AishaBlake merged commit aa5979c into master Mar 13, 2020
23 of 24 checks passed
23 of 24 checks passed
Cloud Tests
Details
TypoCheck Found a few typos
Details
Danger All good
Details
Gatsby Build Service Gatsby Build Service
Details
Gatsby Build Service Gatsby Build Service
Details
Peril All green. Good on 'ya.
Details
build-test Workflow: build-test
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_structured_logging Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
@delete-merged-branch delete-merged-branch bot deleted the lingui branch Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.