[i18n] Remove i18n token skipping from EUI pluralization defString functions by cee-chen · Pull Request #129144 · elastic/kibana

cee-chen · 2022-03-31T21:05:04Z

Summary

This PR handles the pluralization functions in EUI's i18n tokens (see #128825 (comment)). I was hoping to open this PR in after #128825 merged but security tests are being flakey so I'm just going to open this now and rebase later. I did run these tests against all the new EuiSuperDatePicker tokens and can confirm they pass/work as expected.

This PR closes #110132 by correctly checking that the output of EUI's function defStrings output the same strings as i18n's pluralization handling. This requires:

evaling EUI's passed function strings
running the actual i18n.translate util to get final output

Example:

EUI: ({ duration }) => `Last ${duration} second${duration === 1 ? '' : 's'}`
Kibana: 'Last {duration, plural, one {# second} other {# seconds}}'

If `duration` is 1, both should evaluate to `Last 1 second`
If `duration` is 2, both should evaluate to `Last 2 seconds`

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…ana output + add comments/catches for possible future function cases that aren't pluralization

- caught by new unit tests. hooray!

cee-chen · 2022-03-31T21:06:41Z

src/core/public/i18n/i18n_eui_mapping.tsx

    'euiSelectable.searchResults': ({ resultsLength }: EuiValues) =>
      i18n.translate('core.euiSelectable.searchResults', {
-        defaultMessage: '{resultsLength, plural, one {# result} other {# results}}',
+        defaultMessage: '{resultsLength, plural, one {# result} other {# results}} available',


The new tests caught this default message mistake from a few upgrades ago. Hooray! :)

cee-chen · 2022-03-31T21:08:25Z

src/core/public/i18n/i18n_eui_mapping.test.ts

+            throw new Error(
+              `We currently only have logic written for EUI pluralization def functions.
+              This is a new type of def function that will need custom logic written for it.`
+            );


I can't imagine when EUI will ever add function interpolation that isn't for pluralization (see #110132 (comment) to confirm), but if we ever do, I thought it would be prudent to leave this thrown error in for us so that whoever is upgrading the EUI version catches it

cee-chen · 2022-03-31T21:09:39Z

src/core/public/i18n/i18n_eui_mapping.test.ts

+          // pluralization). To check EUI's pluralization against Kibana's pluralization, we
+          // need to eval the defString and then actually i18n.translate & compare the 2 outputs
+          const defFunction = eval(defString); // eslint-disable-line no-eval
+          const defFunctionArg = normalizedDefString.split('({ ')[1].split('})')[0]; // TODO: All EUI pluralization fns currently only pass 1 arg. If this changes in the future and 2 args are passed, we'll need to do some extra splitting by ','


Just want to highlight this TODO - I'm not sure when we would ever pass multiple args into an EUI pluralization fn, but if we ever do in the future this logic will need to be reworked because all I'm essentially doing here is isolating the text in '({ someVar })' to get a single variable name

cee-chen · 2022-03-31T21:10:39Z

src/core/public/i18n/i18n_eui_mapping.test.ts

 const VALUES_REGEXP = /\{\w+\}/;

 describe('@elastic/eui i18n tokens', () => {
+  const i18nTranslateActual = jest.requireActual('@kbn/i18n').i18n.translate;


I wanted this to actually compare final string outputs to each other as I didn't think it was as useful / readable to try and compare the EUI output to some contrived pre-pluralization Kibana version.

afharo

This is great! Thank you for doing this! 🧡 LGTM!

cee-chen · 2022-04-01T03:42:48Z

Thanks for the speedy review @afharo! I'll aim to merge by EOD Friday to give @Bamieh some time to look at this and leave any feedback!

kibana-ci · 2022-04-01T05:03:54Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	309.1KB	309.1KB	+10.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`core`	48	49	+1

Total ESLint disabled count

id	before	after	diff
`core`	58	59	+1

History

💚 Build #35347 succeeded e081a95

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Bamieh

LGTM. Thank you this is very valuable to have in Kibana after the date picker changes

cee-chen · 2022-04-01T15:00:03Z

Thanks y'all!

kibanamachine · 2022-04-05T15:06:19Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 129144 or prevent reminders by adding the backport:skip label.

cee-chen added 2 commits March 31, 2022 13:55

Update tests to compare EUI pluralization function output against Kib…

6c6c603

…ana output + add comments/catches for possible future function cases that aren't pluralization

Fix mismatched EUI/Kibana default message

e081a95

- caught by new unit tests. hooray!

cee-chen requested review from Bamieh and afharo March 31, 2022 21:05

cee-chen requested review from a team as code owners March 31, 2022 21:05

cee-chen added v8.3.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 31, 2022

cee-chen commented Mar 31, 2022

View reviewed changes

cee-chen mentioned this pull request Mar 31, 2022

Upgrade EUI to v53.0.1 #128825

Merged

afharo approved these changes Mar 31, 2022

View reviewed changes

Merge branch 'main' into 110132/i18n-eui-mapping

3084457

Bamieh approved these changes Apr 1, 2022

View reviewed changes

cee-chen merged commit 68630e4 into elastic:main Apr 1, 2022

cee-chen deleted the 110132/i18n-eui-mapping branch April 1, 2022 15:01

kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. backport:skip This PR does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[i18n] Remove i18n token skipping from EUI pluralization defString functions#129144

[i18n] Remove i18n token skipping from EUI pluralization defString functions#129144
cee-chen merged 3 commits intoelastic:mainfrom
cee-chen:110132/i18n-eui-mapping

cee-chen commented Mar 31, 2022

Uh oh!

cee-chen Mar 31, 2022

Uh oh!

cee-chen Mar 31, 2022

Uh oh!

cee-chen Mar 31, 2022 •

edited

Loading

Uh oh!

cee-chen Mar 31, 2022

Uh oh!

afharo left a comment

Uh oh!

cee-chen commented Apr 1, 2022

Uh oh!

kibana-ci commented Apr 1, 2022

ESLint disabled line counts

Total ESLint disabled count

Uh oh!

Bamieh left a comment

Uh oh!

cee-chen commented Apr 1, 2022

Uh oh!

kibanamachine commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

cee-chen commented Mar 31, 2022

Summary

Checklist

Uh oh!

cee-chen Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Apr 1, 2022

Uh oh!

kibana-ci commented Apr 1, 2022

💚 Build Succeeded

Metrics [docs]

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Bamieh left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Apr 1, 2022

Uh oh!

kibanamachine commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cee-chen Mar 31, 2022 •

edited

Loading