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

feat(i18n): enhance types #18347

Merged
merged 11 commits into from
Nov 27, 2024
Merged

feat(i18n): enhance types #18347

merged 11 commits into from
Nov 27, 2024

Conversation

olafsulich
Copy link
Contributor

@olafsulich olafsulich commented Nov 20, 2024

Description

Automatically generates types for internalization file (en-US.json). The script runs on the server start and on a file change (webpack / hot reload). Every change to the en-US.json will cause an update of i18n.d.ts.

The t function now checks for dynamic value types. There's also a unified syntax for passing variables, we eliminate multiple ways (with an object and without) of passing dynamic values. Works with single and multiple values.

What it means?

  • the only allowed variables would be those presented in the translation text
  • ts will let us know when we forgot to pass a variable / pass a wrong one / pass a variable for a text that doesn't have specified one

Before:

// en-US.json
{
 hi: 'hi',
 hello: 'hello {text}'
}

---

t('hi') // ✅ <-- correct

t('hi', 'world') // ✅ <-- incorrect, doesn't throw an error
t('hi', {name: 'world'}) // ✅ <-- incorrect, doesn't throw an error

---

t('hello', 'world') // ✅ <-- correct
t('hello', {text: 'world'}) // ✅ <-- correct

t('hello', {name: 'world'}) // ✅ <-- incorrect, doesn't throw an error
t('hello') // ✅ <-- incorrect, doesn't throw an error

After:

// en-US.json
{
 hi: 'hi',
 hello: 'hello {text}'
}

---

t('hi') // ✅ <-- correct

t('hi', 'world') // ❌ <-- incorrect, throws an error (redundant argument, wrong syntax)
t('hi', {name: 'world'}) // ❌ <-- incorrect, throws an error (redundant argument)

---

t('hello', {text: 'world'}) // ✅ <-- correct

t('hello', 'world') // ❌ <-- incorrect, throws an error (wrong syntax)
t('hello', {name: 'world'}) // ❌ <-- incorrect, throws an error (wrong value "name")
t('hello') // ❌ <-- incorrect, throws an error (missing value)

I've fixed many places to use the new way. I've left some places with @ts-expect-error - I don't want to break anything / it should be done in other PRs.

Checklist

  • mentions the JIRA issue in the PR name (Ex. WPB-423)
  • PR has been self reviewed by the author;
  • Hard-to-understand areas of the code have been commented;
  • If it is a core feature, unit tests have been added;

@@ -0,0 +1,32 @@
const fs = require('fs');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an important file

@@ -108,25 +108,58 @@ const MessageDetails = ({failure, isMessageFocused, allUsers}: MessageDetailsPro
</>
);

const getText = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a lot of changes there, but it had to be done, because of the type, logic remains the same as it was

@@ -19,11 +19,28 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an important file

Comment on lines +166 to +169
identifier: Id,
...args: ExtractSubstitutionKeys<TranslationStrings[Id]> extends never
? [substitutions?: undefined, dangerousSubstitutions?: Record<string, string>, skipEscape?: boolean]
: [substitutions: Substitutions<Id>, dangerousSubstitutions?: Record<string, string>, skipEscape?: boolean]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately, the args are mandatory here (instead of typing each parameter separately), because we have to mark substitutions either optional (the ? sign) with undefined type, or required with the Substitutions<Id> type

@@ -0,0 +1,1654 @@
declare module 'I18n/en-US.json' {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an important file

Copy link
Contributor Author

@olafsulich olafsulich Nov 20, 2024

Choose a reason for hiding this comment

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

i've thought about creating a normal .ts file, but IMO .d.ts is fine in this case

the normal .ts file would require additional import + type assertion the json

@@ -25,6 +25,35 @@ const commonConfig = require('./webpack.config.common');

const srcScript = 'src/script/';

const updateTranslationTypesPlugin = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an important file

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 47.56098% with 43 lines in your changes missing coverage. Please review.

Project coverage is 46.55%. Comparing base (f2c0cf6) to head (38e6b37).
Report is 2 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #18347      +/-   ##
==========================================
- Coverage   46.56%   46.55%   -0.02%     
==========================================
  Files         834      834              
  Lines       26016    26029      +13     
  Branches     5905     5905              
==========================================
+ Hits        12114    12117       +3     
- Misses      12387    12395       +8     
- Partials     1515     1517       +2     

@olafsulich olafsulich marked this pull request as ready for review November 21, 2024 08:01
@olafsulich olafsulich requested review from otto-the-bot and a team as code owners November 21, 2024 08:01
Copy link
Contributor

@przemvs przemvs left a comment

Choose a reason for hiding this comment

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

LGTM!

@olafsulich olafsulich merged commit 084dbf2 into dev Nov 27, 2024
12 checks passed
@olafsulich olafsulich deleted the feat/enhance-i18n-types branch November 27, 2024 06:44
@paulwire paulwire added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: calling comp: preferences echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. 👕 size: XXL type: feature / request ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants