i18n: Support accumulatively registering additional locale data for domain #105

nerrad · 2018-04-10T15:14:54Z

See WordPress/gutenberg#6051 for use-case requiring this change.

codecov · 2018-04-10T15:18:29Z

Codecov Report

Merging #105 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #105   +/-   ##
=======================================
  Coverage   64.54%   64.54%           
=======================================
  Files          41       41           
  Lines         598      598           
  Branches      117      117           
=======================================
  Hits          386      386           
  Misses        170      170           
  Partials       42       42

Impacted Files	Coverage Δ
packages/i18n/src/index.js	`94.73% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d268a2f...aed293b. Read the comment docs.

youknowriad · 2018-04-10T15:34:47Z

packages/i18n/src/index.js

-	i18n.options.locale_data[ domain ] = localeData;
+	i18n.options.locale_data[ domain ] = has( i18n.options.locale_data, domain )
+		? merge( {}, i18n.options.locale_data[ domain ], localeData )
+		: localeData;


Seems reasonable to me, A second opinion might be good. I wonder if we do need the deep merge or if it's possible to use just some Object.assign or similar. I don't have enough knowledge about the locale data format to answer personally.

Also, should we add a unit test?

Got thinking about this, the other concern we might have here is protecting the [""] index in the localeData. We probably don't want plugins to trounce the locale_data[""]['domain'] property right? I'm not sure of what approach should be taken here yet.

It's probably good argument for making sure Jed is ALWAYS instantiated with a domain argument (even if its extracted from locale_data['']['domain'] if it exists and thus i18n.options.locale_data[domain] will remain consistent with locale_data['']['domain']?

We probably don't want plugins to trounce the locale_data[""]['domain'] property right?

What consequences would there be, out of curiosity?

I guess in my head I'm wondering, if a plugin developer erroneously set locale data for the default domain, with the merging behavior would it just be as though they're adding the existing set?

I'd be fine to be explicit with passing the argument, though.

Honestly I'm a bit confused with the behaviour of domain across jed and wp.i18n. There appears to be a lot of inconsistencies. "Domain" is defined:

Within the locale_data itself via localeData['']['domain']

From the explicit domain argument for setLocaleData.

Within locale data as a toplevel property: localeData[domain]

This seems kinda problematic to me, but may just be me not fully grokking the variations there.

Walking through what I understand (mostly for my own benefit), it looks like jed.textdomain is only set from localeData.domain on construct and its simply considered the default value for domain in any i18n function calls when one is not explicitly provided for the function. When code calls somethign like __('some string', 'domain') jed will end up looking for the string in locale_data[ 'domain' ].

So... running through a scenario.

If setLocaleData is called with unique in the domain argument, then regardless of what may be set in localeData['']['domain']... the strings are assigned to locale_data['unique']. Any calls to __('some string') will have the string lookup in localeData['default'], and any calls to __('some string', 'unique') will have the string lookup in localeData['unique'].

However, if in a subsequent call to setLocale there is passed in unique as the domain argument BUT it has this for the shape of the '' index:

localeData[''].domain = 'some_other_domain'

...it looks like 'some_other_domain' is never really referenced anywhere by jed (its just ignored).

So long story short... ya looks like we don't have to worry about the index getting trounced when it comes to the value of localeData[''].domain.

However there is the possibility of a plugin author messing up the plural_forms or lang properties if they are interacting with setLocale directly and not going through an api we add to core. So I'm wondering if we should just add some protection for that, or should we bother? Or maybe its a feature and not an issue because that allows for changing lang for a domain on the fly (however in that case wouldn't we need the options.locale_data[domain] index to be reset)?

...it looks like 'some_other_domain' is never really referenced anywhere by jed (its just ignored).

Interesting. I guess it makes sense, since we're already kinda doing it wrong in Gutenberg:

<script type='text/javascript'> wp.i18n.setLocaleData( {"":{"domain":"gutenberg","lang":"en_US"}} ); </script>

It's assigning data for the default locale (by omitting the second argument), but its own localeData[ '' ].domain is set to gutenberg. Seems it should be default, yeah?

However there is the possibility of a plugin author messing up the plural_forms or lang properties if they are interacting with setLocale directly and not going through an api we add to core. So I'm wondering if we should just add some protection for that, or should we bother?

I'm not entirely clear what you're referring to by "setLocale directly". In any case, I wouldn't overthink it too much, unless it's a footgun.

In any case, the remaining question I have here is if we should do as Riad originally suggested, and use Object.assign to do the merge? Pulling in Lodash's merge could be costly, when we don't need the deep merge.

Seems it should be default, yeah?

Yeah I agree. But since its ignored we probably don't even need to set [''].domain.

I'm not entirely clear what you're referring to by "setLocale directly"

Well the statement I made is on the assumption that something will land in WordPress core that will automate queuing up wp.i18n.setLocale() via wp_inline_script or some other mechanism. But that wouldn't rule out some plugin/code calling wp.i18n.setLocale directly within its own js (as opposed to indirectly through whatever api wp core exposes.

In any case, the remaining question I have here is if we should do as Riad originally suggested, and use Object.assign to do the merge? Pulling in Lodash's merge could be costly, when we don't need the deep merge.

Agreed, and I forgot about Riad's comment (thanks for the reminder). Pushed the change ⚡️

nerrad · 2018-04-10T16:48:21Z

Added some additional unit tests, but I'm new to testing in js (and jest) so not sure if this is an acceptable way to test.

youknowriad · 2018-04-10T17:11:30Z

packages/i18n/src/test/index.js

+//test setting additional localeData
+setLocaleData( additionalLocaleData, 'test_domain' );
+
+describe( 'i18n' , () => {


I guess it's better if describes are unique the i18n is already defined above, I'd rename this one setLocaleData and move it inside the previous i18n describe. And probably wrap setLocaleData( additionalLocaleData, 'test_domain' ); in a beforeAll inside the setLocaleData describe.

Not sure that sentence is clear :P

no the sentence wasn't entirely clear, but after a little googling I think I got the gist of what you wanted?

aduth · 2018-04-10T19:33:46Z

packages/i18n/src/index.js

@@ -34,7 +35,9 @@ export function setLocaleData( localeData = { '': {} }, domain = 'default' ) {
 		} );
 	}

-	i18n.options.locale_data[ domain ] = localeData;
+	i18n.options.locale_data[ domain ] = has( i18n.options.locale_data, domain )


has is a deceptively simple function, one which as used here will invoke a very complex chain of calls to parse the domain argument as a string path (and thus include in the module itself). I think we might be better off using Object#hasOwnProperty here.

ooo did not know that, good to know. I thought has might be different than hasIn and hasIn had the behaviour you described. I'll fix.

aduth · 2018-04-10T19:42:44Z

packages/i18n/src/index.js

-	i18n.options.locale_data[ domain ] = localeData;
+	i18n.options.locale_data[ domain ] = has( i18n.options.locale_data, domain )
+		? merge( {}, i18n.options.locale_data[ domain ], localeData )
+		: localeData;


We probably don't want plugins to trounce the locale_data[""]['domain'] property right?

What consequences would there be, out of curiosity?

I guess in my head I'm wondering, if a plugin developer erroneously set locale data for the default domain, with the merging behavior would it just be as though they're adding the existing set?

I'd be fine to be explicit with passing the argument, though.

nerrad · 2018-04-10T23:45:11Z

packages/i18n/src/index.js

@@ -35,7 +35,7 @@ export function setLocaleData( localeData = { '': {} }, domain = 'default' ) {
 		} );
 	}

-	i18n.options.locale_data[ domain ] = has( i18n.options.locale_data, domain )
+	i18n.options.locale_data[ domain ] = domain in i18n.options.locale_data


so @aduth I ended up using in because it appears that's the most performant option

Well to be more accurate, its the most performant option in firefox and chrome. In Safari hasOwnProperty wins. I'm happy to use whatever you prefer.

It's important to understand that in has different behavior than hasOwnProperty though, where the former traverses up through the prototype chain of the object.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in

As implemented here, this is susceptible to errors / oddities when doing something like assigning valueOf as a domain, or any other member of the object prototype.

'valueOf' in {}; // true ( {} ).hasOwnProperty( 'valueOf' ); // false

This line of code is likely to be called only a handful of times for an entire page session, so optimizing for performance here is a bit overkill.

Related: #85

gotcha, makes sense, changed it.

aduth · 2018-04-12T18:55:08Z

packages/i18n/src/index.js

@@ -34,7 +35,9 @@ export function setLocaleData( localeData = { '': {} }, domain = 'default' ) {
 		} );
 	}

-	i18n.options.locale_data[ domain ] = localeData;
+	i18n.options.locale_data[ domain ] = i18n.options.locale_data.hasOwnProperty(domain)


Code style†: Space between parentheses (reference

† This repository is not (yet) enforced by ESLint standards.

aduth · 2018-04-12T18:57:09Z

packages/i18n/src/test/index.js

@@ -26,6 +26,10 @@ const localeData = {

 	"fruit\u0004%d apple" : [ "une pomme", "%d pommes" ],
 }
+const additionalLocaleData = {
+	"cheeseburger" : ["hamburger au fromage"]


Code style:

Space between array square brackets (reference)

No space before property colon (reference)

No quotes on property key when not needed (reference)

Single quote for strings (reference)

aduth · 2018-04-12T18:57:35Z

packages/i18n/src/test/index.js

@@ -84,4 +88,21 @@ describe( 'i18n', () => {
 			expect( result ).toBe( 'bonjour Riad' );
 		} );
 	} );
+
+	describe( 'setAdditionalLocale' , () => {


Code style: No space before comma (reference)

aduth · 2018-04-12T19:49:52Z

packages/i18n/src/index.js

@@ -34,7 +34,9 @@ export function setLocaleData( localeData = { '': {} }, domain = 'default' ) {
 		} );
 	}

-	i18n.options.locale_data[ domain ] = localeData;
+	i18n.options.locale_data[ domain ] = i18n.options.locale_data.hasOwnProperty( domain )


Okay, one more question 😄

Do we care about mutating i18n.options.locale_data ?

Could skip an object clone to drop the first {} argument of Object.assign and assign directly.

Maybe as:

if ( i18n.options.locale_data.hasOwnProperty( domain ) ) { Object.assign( i18n.options.locale_data[ domain ], localeData ) } else { i18n.options.locale_data[ domain ] = localeData; }

Direct assignment with = is fine too, since Object.assign returns the resulting target object.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

Or: We could simplify this to always assign into the {}, even if ! i18n.options.locale_data.hasOwnProperty( domain ), since it's fine if anything but the first argument of Object.assign is undefined:

i18n.options.locale_data[ domain ] = Object.assign( {}, i18n.options.locale_data[ domain ], localeData );

Hmm I'm fine either way. I wasn't sure if we should mutate the object directly or not but in my case its likely a force of habit working with react/redux that I just merge into a new object. So mutation should be okay here.

I like your last example so I'll roll with it.

aduth

Good to go after tabbing added.

aduth · 2018-04-12T20:18:41Z

packages/i18n/src/index.js

@@ -34,7 +34,11 @@ export function setLocaleData( localeData = { '': {} }, domain = 'default' ) {
 		} );
 	}

-	i18n.options.locale_data[ domain ] = localeData;
+i18n.options.locale_data[ domain ] = Object.assign(


These lines need to be tabbed once more.

allow for setting additional locale data for a domain accumulatively

601b638

nerrad mentioned this pull request Apr 10, 2018

Extract map for i18n strings to use for directed load of i18n strings per view. WordPress/gutenberg#6051

Closed

6 tasks

nerrad changed the title ~~i18n: Support accumutively registering additional locale data for domain~~ i18n: Support accumulatively registering additional locale data for domain Apr 10, 2018

woops wrong object reference

b3775ae

youknowriad reviewed Apr 10, 2018

View reviewed changes

add test for accumulation of localeData

1e430cf

youknowriad reviewed Apr 10, 2018

View reviewed changes

repackage additional tests

0757767

aduth reviewed Apr 10, 2018

View reviewed changes

use more performant “in” check instead of lodash _.has

3bb5b9c

nerrad commented Apr 10, 2018

View reviewed changes

use hasOwnProperty instead of in

9a6d78e

aduth reviewed Apr 12, 2018

View reviewed changes

nerrad added 4 commits April 12, 2018 15:07

fix formatting for code style

3f56469

fix formatting

25c686f

use Object.assign instead of lodash.merge

8dfe686

add new plural strings to tests

a39924a

aduth reviewed Apr 12, 2018

View reviewed changes

simplify merging

533c64e

aduth reviewed Apr 12, 2018

View reviewed changes

nerrad and others added 2 commits April 12, 2018 16:20

indent, indent

1b31bc8

i18n: Fix indentation

aed293b

aduth added the [Package] i18n label Apr 12, 2018

aduth approved these changes Apr 12, 2018

View reviewed changes

aduth merged commit ed56276 into WordPress:master Apr 12, 2018

nerrad deleted the accumulative-i18n-locale branch April 12, 2018 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: Support accumulatively registering additional locale data for domain #105

i18n: Support accumulatively registering additional locale data for domain #105

nerrad commented Apr 10, 2018

codecov bot commented Apr 10, 2018 •

edited

Loading

youknowriad Apr 10, 2018

nerrad Apr 10, 2018 •

edited

Loading

nerrad Apr 10, 2018 •

edited

Loading

aduth Apr 10, 2018

nerrad Apr 10, 2018 •

edited

Loading

aduth Apr 12, 2018

nerrad Apr 12, 2018

nerrad commented Apr 10, 2018

youknowriad Apr 10, 2018

nerrad Apr 10, 2018

aduth Apr 10, 2018

nerrad Apr 10, 2018

aduth Apr 10, 2018

nerrad Apr 10, 2018

nerrad Apr 10, 2018

aduth Apr 11, 2018

aduth Apr 11, 2018

nerrad Apr 11, 2018

aduth Apr 12, 2018

aduth Apr 12, 2018

aduth Apr 12, 2018

aduth Apr 12, 2018

nerrad Apr 12, 2018

aduth left a comment

aduth Apr 12, 2018

i18n: Support accumulatively registering additional locale data for domain #105

i18n: Support accumulatively registering additional locale data for domain #105

Conversation

nerrad commented Apr 10, 2018

codecov bot commented Apr 10, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nerrad Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

nerrad Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 10, 2018 •

edited

Loading

nerrad Apr 10, 2018 •

edited

Loading

nerrad Apr 10, 2018 •

edited

Loading

nerrad Apr 10, 2018 •

edited

Loading