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

Use next-intl for internationalization, replacing i18next #260

Merged
merged 15 commits into from
Dec 6, 2023
Merged

Conversation

sawyerh
Copy link
Contributor

@sawyerh sawyerh commented Dec 6, 2023

Ticket

Part of #66

Changes

  • Replace all I18next dependencies with next-intl
  • Add new tests/test-utils file that would be used for rendering and querying a React tree in tests, rather than directly importing from @testing-library/react. This is so that the i18n content is provided to the component being tested.

Context for reviewers

This is one of the main pieces of the larger migration effort from the Pages router to the App Router (#66). To reduce the size of that PR, I've broken out the i18n migration work into this PR.

Next.js App Router doesn't support internationalized routing or locale detection, like Pages router, and next-intl makes it easy to restore that functionality via middleware.

Testing

CleanShot 2023-12-05 at 16 02 51

CleanShot 2023-12-05 at 16 09 08

Copy link
Contributor Author

Choose a reason for hiding this comment

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

❓ Would it be useful to go ahead and demonstrate separation of content files, like a en-US/components.ts and en-US/pages.ts?

Copy link
Contributor

Choose a reason for hiding this comment

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

seems useful. it'll make it easier to separate out files that we don't expect projects to change much and files that projects are expected to change, which will make it easier for projects to pull in updates from the template, and also make it easier to explore our idea of templates being built off of other templates.

don't feel strongly about it being in this PR, doing it later, or continuing to discuss first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think most projects would modify both components and pages locale files

Copy link
Contributor

Choose a reason for hiding this comment

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

i was thinking more like there'd be something like "core components" (e.g. /components/core) like button, etc that shouldn't get modified much (and eventually if they get really stable, we could make it a library), and projects of course would make their own project-specific components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking about this some more and I think a single messages file per language may be a better initial recommendation (what got merged), since I think our other recommendation is to use a translation management system much earlier in a project. The separation of messages files into pages (or a file per page) and components is mainly something I imagine teams do to make it easier to manually manage translations in the JSON files. But if teams are using a translation management system instead, then the separation of files is less necessary (and maybe causes more work when pulling the messages into the codebase).

@sawyerh sawyerh changed the title Replace i18next with next-intl Use next-intl for internationalization, replacing i18next Dec 6, 2023
@sawyerh sawyerh marked this pull request as ready for review December 6, 2023 00:17
@@ -14,35 +15,37 @@ module.exports = {
// dependencies to work in standalone mode. It may be overkill for most projects at
// Nava which aren't image heavy.
"@next/next/no-img-element": "off",
"no-restricted-imports": [
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@@ -24,12 +21,11 @@
"dependencies": {
"@trussworks/react-uswds": "^6.0.0",
"@uswds/uswds": "3.7.0",
"i18next": "^23.0.0",
"lodash": "^4.17.21",
Copy link
Contributor

Choose a reason for hiding this comment

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

did we add lodash w/ this PR or is the diff just weird? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it in this PR. Its merge method is used for merging the default locale to provide fallbacks for missing content in other locales. Its pick method would also be eventually used for picking specific content to send into a client-side component (a follow up App router migration PR will demonstrate that).

Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

big PR so i didn't review in detail but ali reviewed it and it also looks good to me at a high level. left some comments

@@ -1,4 +1,4 @@
import { useTranslation } from "next-i18next";
import { useTranslations } from "next-intl";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could we re-export useTranslations within the src/i18n module and import useTranslations from src/i18n rather than directly importing the library?

Might feel nitpicky but generally except in some cases for framework dependencies (i.e. Next.js) or very light utility dependencies like lodash, I prefer to wrap all dependencies in our own module and re-export only the functionality we want to use.

The rationale is that we generally want to control our dependencies, and when we expand our dependencies we want that decision to be an explicit one that is reviewed and critiqued rather than one that a single engineer can accidentally slip by. For adding entire libraries, this is caught by changes to package.json — if a reviewer sees a new package in package.json that wasn't previously discussed, they'll easily catch that and it's an opportunity to discuss the addition of the dependency. Within a package, there is typically a lot of functionality since libraries are designed to be maximally flexible and support a lot of use cases across a variety of projects. However, on our specific project, you typically only want a specific subset of that functionality, and you want any changes to the subset of functionality you depend on to be an explicit decision. If the pattern is for engineers to import libraries directly, then it's very easy to start using functionality in the library that the original engineer that added the library didn't intend to use. Therefore, I think it's best to explicitly wrap the functionality you want in a wrapper module, and have engineers only import the wrapper module. This way, if an engineer wants to use additional functionality from the library, they'll have to explicitly add it to the wrapper module, and that will be more easily caught in review to ensure that it's an explicit decision to do that (and incur the associated cost of expanding the surface area of dependencies).

For example it could be like

// i18n.js
export { useTranslations } from "next-intl";

then here we'd do
import {useTranslations} from "../src/i18n" instead of from "next-intl"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yea I've debated doing this in the past when using i18next too. If it's cool with you, I may explore doing this in a follow-up when App Router is in place. An open question I have is what the impact is on client-side bundle size if a client component imports from src/i18n, where all of the locale files are also being imported. What we wouldn't want is all of those locale strings being included in the client component's bundle. This would be easier for me to analyze once the app is utilizing server components (app router).

Copy link
Contributor

Choose a reason for hiding this comment

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

optional nit: naming is hard but it'd be nice to name this something more specific to its use case. e.g. if it's gonna be used for testing any react components, then it could be react-testing-utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed from tests/test-utils to tests/react-utils (removed "test" since that felt implicit being in the tests directory already)

@sawyerh sawyerh merged commit ed37131 into main Dec 6, 2023
6 checks passed
@sawyerh sawyerh deleted the 66-next-intl branch December 6, 2023 22:58
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.

3 participants