Refactored @wordpress/i18n to be an instantiable

[jameslnewell]

Description

As per the proposed solution in #19210 I've refactored the @wordpress/i18n module to expose an instantiable class. e.g.

import { I18N } from '@wordpress/i18n';

const strayaLocale = {
	hello: [ 'gday' ],
};

const frenchLocale = {
	hello: [ 'bonjour' ],
};

const straya = new I18N();
const french = new I18N();

straya.setLocaleData( strayaLocale );
french.setLocaleData( frenchLocale );

expect( straya.__( 'hello' ) ).toEqual( 'gday' );
expect( french.__( 'hello' ) ).toEqual( 'bonjour' );

There are no changes to the existing external interface of the module other than the addition of the I18N class.

How has this been tested?

Via the existing suite of unit tests and an additional unit test.

Screenshots

N/A

Types of changes

New feature.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• 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.

[sirreal]

🤦‍♂ I just started on this in #20332

[sirreal]

I don't mind which PR moves ahead, this or #20332. However, I suspect the structure #20332 will lead to better tree shaking. Apps that don't use the default instance via importing translate functions (__) directly shouldn't pay the cost.

[jameslnewell]

However, I suspect the structure #20332 will lead to better tree shaking. Apps that don't use the default instance via importing translate functions (__) directly shouldn't pay the cost.

@sirreal Are you saying tree shaking won't work unless I use the .bind(x) approach? I thought tree shaking worked fine with the current implementation when only importing the instantiable
class ??

[sirreal]

Are you saying tree shaking won't work unless I use the .bind(x) approach?

No, I'm saying that if the index only re-exports from other modules, we'll get better tree shaking. As it is import { I18n } from '@wordpress/i18n will result in a default instance being instantiated even if its never used. If the default bits move out of the index they become candidates for tree shaking.

This module structure seems ideal (see #20332):

• index (rexports)
• sprintf
• i18n class
• default i18n instance + exposed methods from instance

[jameslnewell]

I've made the changes as desired but as per my example in the rollup repl I'm pretty confident that the exports were already tree shakable and that the refactor won't result in an improvement to what gets outputted.

[sirreal]

Overall this looks great, thanks! I've left some notes, but there are no major blockers.

I am concerned about effectively loosing all the documentation for the module. It's important to find a solution for that before this can be released.

[jameslnewell]

I am concerned about effectively loosing all the documentation for the module. It's important to find a solution for that before this can be released.

💯. I think the short term solution is to duplicate the JS docs (or because the tool doesn't document the class anyway, move the documentation to the module methods which do get documented by the tool). Longer term it might be worth looking at a tool that has a few more smarts e.g. typedoc or traversing the TS AST - I think thats well beyond the scope of this PR though.

[jsnajdr]

I've made the changes as desired but as per my example in the rollup repl I'm pretty confident that the exports were already tree shakable and that the refactor won't result in an improvement to what gets outputted.

Rollup and webpack are different bundlers and their tree shaking algorithms are different. The same example won't be tree shaked in webpack.

The Rollup algorithm seems to be more aggressive and it's looking at more things. For example, in the unoptimized program, the I18N constructor would be called twice. Rollup looks at the actual constructor and if it's empty, it knows that it can optimize the unused new I18N out.

But once I add a console.log statement to the constructor, Rollup doesn't optimize and keeps both new I18N calls in the bundle!

Webpack's tree shaking is much simpler. It only looks at the import and export statements and the binding names they use. The actual JS code is mostly invisible to webpack.

[sirreal]

Longer term it might be worth looking at a tool that has a few more smarts […] I think thats well beyond the scope of this PR though.

I agree, it's not trivial: #18045

The best way for now may simply be to reproduce the types in the index:

/**
 * Merges locale data into the Tannin instance by domain. Accepts data in a
 * Jed-formatted JSON object shape.
 *
 * @see http://messageformat.github.io/Jed/
 *
 * @param {LocaleData} [data]   Locale data configuration.
 * @param {string}     [domain] Domain for which configuration applies.
 */
export { setLocaleData } from './default-i18n';

In my testing, that works well enough.

[sirreal]

We'll need to rebase this and add the isRTL method from #20298.

[aduth]

We'll need to rebase this and add the isRTL method from #20298.

I think it'd also be nice to use this opportunity to refactor the tests added in #20298 to avoid manipulating the require cache and instead create new instances between tests.

These changes:

https://github.com/WordPress/gutenberg/pull/20298/files#diff-acf9737fbf7d5643d202c6452a6dc11eL34-R35

[jameslnewell]

think it'd also be nice to use this opportunity to refactor the tests added in #20298 to avoid manipulating the require cache and instead create new instances between tests.

@aduth Do you have any opinions around the tests? Do you think we should keep test coverage on both, the i18n instance and the default i18n module methods or do you think its redundant and we should only test the i18n instance and accept the small risk of the i18n module methods becoming out-of-sync?

[aduth]

I don't think we need to repeat the same tests for the default methods. It should be enough to test methods on an instance created by createI18n.

[aduth]

Should we always be calling this, even if initialData and/or initialDomain are possibly undefined (per the documented types)?

[aduth]

Related to this, are there specific use-cases in mind for setting the initial data? It seems like something which could be useful, but just wondering if we might be getting ahead of ourselves.

[sirreal]

I requested this. It seems helpful to be able to populate with initial data, but there's not much lost if we require package consumers to first create then set the data. I don't have strong feelings.

[aduth]

It's still odd to me that we call setLocaleData if there is nothing to set. It works, but only by a combination of the fact that data is an optional argument of setLocaleData (pretty counter-intuitive), and that object spread is tolerant of undefined (i.e. { ...undefined }), when instead we could just add an if here, and make setLocaleData's data argument non-optional.

Suggested change

	setLocaleData( initialData, initialDomain );
	if ( initialData ) {
	setLocaleData( initialData, initialDomain );
	}

[sirreal]

@jameslnewell Do you mind addressing this?

[sirreal]

I addressed this in #21182. I believe that's the remaining feedback that needed to be addressed.

[sirreal]

This works well in my testing.

I don't like that we've moved away from a class which nicely encapsulated state and behavior, but if that's the project convention and preference I'll concede. The end result isn't all that different.

When considering types, now we'll likely end up accessing them as something along the lines of:

// Create function
type __ = ReturnType< typeof createI18n >['__'];
// Class
type __ = I18n['__']

Would we consider using a class internally and exposing a create function that wraps it like was suggested here #20318 (comment)

const createI18n = ( ...args ) => new I18n( ...args );

[sirreal]

I'd like to get this merged, it seems to be in good shape so I'll plan to land it early next week unless there's additional feedback.

cc: @youknowriad @aduth @jsnajdr who've been actively involved in discussion.

[aduth]

Would we consider using a class internally and exposing a create function that wraps it like was suggested here #20318 (comment)

const createI18n = ( ...args ) => new I18n( ...args );

Is the idea that the type of the class I18n would be available, but not the implementation itself?

[sirreal]

Would we consider using a class internally and exposing a create function that wraps it like was suggested here #20318 (comment)

const createI18n = ( ...args ) => new I18n( ...args );

Is the idea that the type of the class I18n would be available, but not the implementation itself?

I did some testing and this may not be an issue because the type is defined independently. With #18942, it may require this change in the index to export the I18n and LocaleData types as well:

export * from './create-i18n';

We should be able to sort this out after, I'm not concerned about getting this change into this PR.

[sirreal]

This appears to be abandoned. I've created #21182 to address feedback as I can't work on this branch.

[aduth]

Closing in favor of #21182.