Skip to content
This repository has been archived by the owner on Jul 9, 2018. It is now read-only.

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

Merged
merged 13 commits into from
Apr 12, 2018
Merged

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

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
601b638
allow for setting additional locale data for a domain accumulatively
nerrad Apr 10, 2018
b3775ae
woops wrong object reference
nerrad Apr 10, 2018
1e430cf
add test for accumulation of localeData
nerrad Apr 10, 2018
0757767
repackage additional tests
nerrad Apr 10, 2018
3bb5b9c
use more performant “in” check instead of lodash `_.has`
nerrad Apr 10, 2018
9a6d78e
use hasOwnProperty instead of in
nerrad Apr 11, 2018
3f56469
fix formatting for code style
nerrad Apr 12, 2018
25c686f
fix formatting
nerrad Apr 12, 2018
8dfe686
use Object.assign instead of lodash.merge
nerrad Apr 12, 2018
a39924a
add new plural strings to tests
nerrad Apr 12, 2018
533c64e
simplify merging
nerrad Apr 12, 2018
1b31bc8
indent, indent
nerrad Apr 12, 2018
aed293b
i18n: Fix indentation
aduth Apr 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/i18n/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import Jed from 'jed';
import memoize from 'memize';
import { merge, has } from 'lodash';
import { merge } from 'lodash';

let i18n;

Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Related: #85

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, makes sense, changed it.

? merge( {}, i18n.options.locale_data[ domain ], localeData )
: localeData;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@nerrad nerrad Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nerrad nerrad Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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']?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nerrad nerrad Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. Within the locale_data itself via localeData['']['domain']
  2. From the explicit domain argument for setLocaleData.
  3. 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)?

Copy link
Member

Choose a reason for hiding this comment

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

...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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ⚡️

}
Expand Down