Skip to content

Conversation

sawyerh
Copy link
Contributor

@sawyerh sawyerh commented Aug 19, 2023

Ticket

Resolves #194

Changes

  • Add a new i18n-types NPM script for generating a TypeScript declaration of the JSON English locale file(s)
  • Configure i18next to be type-safe, to prevent references to i18n key paths that don't exist in the locale files

Context for reviewers

I'm interested to get feedback on this proposal. It comes with some tradeoffs...

Pros

  • Type-safe i18n! This is useful for catching translation errors early.
  • In addition to type safety, it also enables autocomplete capability in IDE's (see demo below).
  • It's not always clear to teams how to enable type-safety of i18next, so providing this out-of-the-box could be helpful for teams that want it.

Cons

  • This relies on a TypeScript file that references every English JSON file. I can definitely see an engineer adding a new English locale file and getting TypeScript errors when they attempt to reference keys in it, because they're already running the development server and didn't know to run npm run i18n-types.
  • I don't love that this relies on a new dependency to generate the TypeScript file. Instructing engineers in the README to manually update this file also didn't feel like a great option.
  • Like other TypeScript errors, the type errors that get shown for invalid keys don't make it obvious that the error is due to the key not existing in the locale definition. There is a nice feature where it can sometimes include a "Did you mean" suggestion though (see demo below).

I think the benefits of type-safe i18n is worth this added potential for confusion, but I'm curious to get people's thoughts on that.

Testing

CleanShot.2023-08-18.at.19.10.49.mp4

Example of the test coverage to help teams identify that the file needs to be updated:

CleanShot 2023-08-19 at 10 10 01@2x

@sawyerh sawyerh requested review from lorenyu and a team August 19, 2023 02:13
@sawyerh sawyerh requested review from aligg and emilycnava August 19, 2023 02:20
@@ -20,7 +17,7 @@ const primaryLinks: {
i18nKey: "nav_link_health",
href: "/health",
},
];
] as const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ You can't pass in a string type to t(). This makes it so i18nKey is "nav_link_home" | "nav_link_health"

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.

Looks great. At first I was concerned that this approach doesn't work for content bundles that are stored separately from the source code, but after skimming this article looks like there's a way to handle that.

I'll think on this a bit longer but so far I'm supportive of it.

@@ -122,6 +123,7 @@ npm run test-watch -- pages

- [TypeScript](https://www.typescriptlang.org/) is used for type checking.
- `npm run ts:check` - Type checks all files
- `npm run i18n-types` - Updates the i18n TypeScript declaration. You only need to run this if you've added a new English locale file (JSON files in `public/locales/en`). This runs automatically when you start the development server or build the application.
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely a "gotcha" but I think since we should be adding new files relatively rarely it's worth it.

@lorenyu
Copy link
Contributor

lorenyu commented Aug 21, 2023

some non-blocking thoughts for discussion:

thought: out of scope for this PR but i'm wondering how this solution would/wouldn't need to change once we add in external content management systems for managing the translation bundles e.g. like Phrase or Locize or something.

another thought: there may/may not be value in an ADR outline the pros/cons of this type-checking approach vs an approach using i18next's missing key error functionality to do a unit test that checks for non-existent keys. i don't feel strongly about it, i feel like the design decision here to rely on static type checking is reasonably "obvious" enough to not require an ADR.

final thought: i wonder if there's a way to automatically check for unused keys so that we can help with cleanup of deprecated content keys.

sawyerh added a commit that referenced this pull request Aug 29, 2023
## Ticket

Relates to #194 

## Context for reviewers

An implementation can be seen at
#199
@sawyerh sawyerh merged commit 97a0a4b into main Aug 29, 2023
@sawyerh sawyerh deleted the sawyerh/194-ts-i18n branch August 29, 2023 22:45
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.

Make i18next type-safe
3 participants