i18n: Create new function `addLocaleData` to merge domain configuration #37704

arthur791004 · 2022-01-04T09:26:23Z

Description

As #34101 and #37686 mentioned, calling setLocaleData multiple times, the domain configuration will be overwritten by the latest one. It might be better to merge the domain configuration rather than overwrite. However, we should avoid any breaking change. So, create a new function addLocaleData to merge the domain configuration and keep the behavior of setLocaleData the same.

How has this been tested?

Go to http://localhost:8888/wp-admin/post.php?post=1&action=edit
Open Developer Tool

Do the following commands

wp.i18n.addLocaleData({ "": { lang: 'en' } })
wp.i18n.addLocaleData({ "": {} })
wp.i18n.getLocaleData()[""]

Output
```
{ lang: "en", plural_forms: f } 
```

Types of changes

New feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

sirreal · 2022-01-13T12:50:32Z

👋 Thanks for pinging me, @swissspidy.

I haven't been working with this package lately. I'm not in a good position to make decisions about introducing new APIs.

Once we're confident we'd like to introduce a new API, I'm happy to help with code review.

jsnajdr

This is a great addition, thank you! The new API makes sense.

There's a pre-existing little bug in both setLocaleData and addLocaleData: as they also support updating the locale "metadata" under the '' key, they can also update the [ '' ][ 'plural-forms' ] key. But the tannin library internally caches that. Both functions should therefore do:

tannin.pluralForms[ domain ] = null;

to force updating the cache. That would be nice to fix, either here or in a follow-up PR.

jsnajdr · 2022-01-13T13:34:44Z

packages/i18n/src/create-i18n.js

+	 */
+	const doAddLocaleData = ( data, domain = 'default' ) => {
+		tannin.data[ domain ] = {
+			...DEFAULT_LOCALE_DATA,


The ...DEFAULT_LOCALE_DATA spread is redundant, because it has only the '' key and that's gurananteed to be completely overwritten by the '' key few lines later. The doSetLocaleData function has the same redundancy.

jsnajdr · 2022-01-13T13:36:08Z

packages/i18n/src/create-i18n.js

+			'': {
+				...DEFAULT_LOCALE_DATA[ '' ],
+				...( tannin.data[ domain ] || {} )[ '' ],
+				...( data || {} )[ '' ],


You can use optional chaining for this: ...tannin.data[ domain ]?.[ '' ]

jsnajdr · 2022-01-13T13:38:08Z

packages/i18n/src/create-i18n.js

 	/** @type {SetLocaleData} */
 	const setLocaleData = ( data, domain ) => {
 		doSetLocaleData( data, domain );
 		notifyListeners();
 	};

+	/** @type {AddLocaleData} */
+	const addLocaleData = ( data, domain ) => {
+		doAddLocaleData( data, domain );


The doAddLocaleData function could be inlined here. There is a standalone doSetLocaleData function because we sometimes want to do the operation without firing notifications, but addLocaleData doesn't have that requirement.

Now you can remove doAddLocaleData completely, and do the tannin assignment directly in the addLocaleData body.

arthur791004 · 2022-01-14T06:41:28Z

@jsnajdr Thanks for your review! I've fixed pluralForms cache issue and made some changes according to your feedback 👍

As the value of tannin.pluralForms[ domain ] should be function, I deleted the value to force updating the cache.

jsnajdr

Thanks for addressing the feedback, the PR looks good 👍 Do you need any help with merging or do you have permission to do that yourself.

There are e2e failures in editor/blocks/navigation.test.js -- are they possibly related? The test fails to find an element by it text content, where the text is produced by __(), so __() malfunction is a possible reason for failure.

jsnajdr · 2022-01-14T10:29:30Z

packages/i18n/src/create-i18n.js

 	/** @type {SetLocaleData} */
 	const setLocaleData = ( data, domain ) => {
 		doSetLocaleData( data, domain );
 		notifyListeners();
 	};

+	/** @type {AddLocaleData} */
+	const addLocaleData = ( data, domain ) => {
+		doAddLocaleData( data, domain );


Now you can remove doAddLocaleData completely, and do the tannin assignment directly in the addLocaleData body.

arthur791004 · 2022-01-14T11:44:55Z

Do you need any help with merging or do you have permission to do that yourself.

@jsnajdr Oh yes, I need help to merge this one as I don't have permission. Thanks!

There are e2e failures in editor/blocks/navigation.test.js -- are they possibly related?

I don't think the e2e failures are related to this one as everything keeps the same except removing redundant
spread and fixing the cache issue. But I think both of them won't affect the e2e testing 🤔

swissspidy · 2022-01-14T12:41:55Z

A changelog entry for this new function would be great

arthur791004 · 2022-01-17T02:50:02Z

A changelog entry for this new function would be great

@swissspidy I've updated the changelog, thanks 👍

jsnajdr · 2022-01-17T09:44:53Z

packages/i18n/CHANGELOG.md

@@ -2,6 +2,8 @@

 ## Unreleased

+- Add new `addLocaleData` method to merges locale data into the Tannin instance by domain. Note that this function will also merges the domain configuration.
+


Let's fix the typo in "merges" which should read as "merge". And I would personally remove the "Note" because a changelog entry doesn't need this level of detail about the new method.

The PR will be finally ready to land after this last fixup is done.

Oops...I'm sorry for the typo 😅.

And I would personally remove the "Note" because a changelog entry doesn't need this level of detail about the new method.

Okay 👍

arthur791004 requested a review from swissspidy as a code owner January 4, 2022 09:26

arthur791004 mentioned this pull request Jan 4, 2022

i18n: Merge domain configuration when calling setLocaleData #37686

Closed

7 tasks

arthur791004 force-pushed the feat/add-locale-data branch from d7ac53c to b918f09 Compare January 4, 2022 09:38

amustaque97 added the [Package] i18n /packages/i18n label Jan 7, 2022

swissspidy requested review from sirreal and jsnajdr January 7, 2022 19:03

jsnajdr approved these changes Jan 13, 2022

View reviewed changes

p-jackson mentioned this pull request Jan 14, 2022

Importer: Fixes direction of import URL capture button in production Automattic/wp-calypso#60068

Merged

jsnajdr approved these changes Jan 14, 2022

View reviewed changes

autumnfjeld mentioned this pull request Jan 17, 2022

[Core] Avoid empty translation to overwrite the domain configuration Automattic/wp-calypso#60111

Closed

arthur791004 added 8 commits January 17, 2022 11:02

i18n: Add addLocaleData function

0b751cb

i18n: Add test case for addLocaleData

7b67af5

Remove redundant spread

c0ca0d3

Change to use optional chaining

3dddd55

Make doAddLocaleData an inline function

50d850a

Clean up pluralForms cache

9562e20

Remove doAddLocaleData function

cbf5b61

Update changelog files

0c306d7

arthur791004 force-pushed the feat/add-locale-data branch from 14eabc3 to 0c306d7 Compare January 17, 2022 03:02

jsnajdr reviewed Jan 17, 2022

View reviewed changes

Fix typo and update changelog

85138c1

swissspidy approved these changes Jan 17, 2022

View reviewed changes

swissspidy merged commit ae55b94 into WordPress:trunk Jan 17, 2022

github-actions bot added this to the Gutenberg 12.5 milestone Jan 17, 2022

arthur791004 deleted the feat/add-locale-data branch January 27, 2022 06:58

arthur791004 mentioned this pull request Feb 15, 2022

[Discussion] i18n: the domain configuration will be overwritten by setLocaleData under @wordpress/i18n #34101

Closed

2 tasks

arthur791004 mentioned this pull request May 4, 2022

ETK: Cleanup unused copy/pasted script data Automattic/wp-calypso#62897

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: Create new function `addLocaleData` to merge domain configuration #37704

i18n: Create new function `addLocaleData` to merge domain configuration #37704

arthur791004 commented Jan 4, 2022 •

edited

Loading

sirreal commented Jan 13, 2022

jsnajdr left a comment

jsnajdr Jan 13, 2022

jsnajdr Jan 13, 2022

jsnajdr Jan 13, 2022

jsnajdr Jan 14, 2022

arthur791004 commented Jan 14, 2022

jsnajdr left a comment

jsnajdr Jan 14, 2022

arthur791004 commented Jan 14, 2022 •

edited

Loading

swissspidy commented Jan 14, 2022

arthur791004 commented Jan 17, 2022 •

edited

Loading

jsnajdr Jan 17, 2022

arthur791004 Jan 17, 2022

		@@ -2,6 +2,8 @@

		## Unreleased

		- Add new `addLocaleData` method to merges locale data into the Tannin instance by domain. Note that this function will also merges the domain configuration.

i18n: Create new function addLocaleData to merge domain configuration #37704

i18n: Create new function addLocaleData to merge domain configuration #37704

Conversation

arthur791004 commented Jan 4, 2022 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

sirreal commented Jan 13, 2022

jsnajdr left a comment

Choose a reason for hiding this comment

jsnajdr Jan 13, 2022

Choose a reason for hiding this comment

jsnajdr Jan 13, 2022

Choose a reason for hiding this comment

jsnajdr Jan 13, 2022

Choose a reason for hiding this comment

jsnajdr Jan 14, 2022

Choose a reason for hiding this comment

arthur791004 commented Jan 14, 2022

jsnajdr left a comment

Choose a reason for hiding this comment

jsnajdr Jan 14, 2022

Choose a reason for hiding this comment

arthur791004 commented Jan 14, 2022 • edited Loading

swissspidy commented Jan 14, 2022

arthur791004 commented Jan 17, 2022 • edited Loading

jsnajdr Jan 17, 2022

Choose a reason for hiding this comment

arthur791004 Jan 17, 2022

Choose a reason for hiding this comment

i18n: Create new function `addLocaleData` to merge domain configuration #37704

i18n: Create new function `addLocaleData` to merge domain configuration #37704

arthur791004 commented Jan 4, 2022 •

edited

Loading

arthur791004 commented Jan 14, 2022 •

edited

Loading

arthur791004 commented Jan 17, 2022 •

edited

Loading