Update provider so the `useI18n()` hook can use the global translation functions from `@wordpress/i18n` #47352

p-jackson · 2020-11-12T04:56:03Z

Changes proposed in this Pull Request

@automattic/react-i18n works by providing locale data somewhere near the root of your component tree (using <I18nProvider>), and using the useI18n() hook in your components to get access to translation functions that use the provided locale data.

However we don't always have access to the locale data. In the ETK, core initialises @wordpress/i18n with the locale data and we don't get access to it. That means __ from @wordpress/i18n works, but __ from useI18n() does not.

This PR ~~adds a <GlobalI18nProvider> provider that is useful~~ changes the <I18nProvider> provider so it is useful in situations where we don't have access to the raw locale data. When this provider is used, useI18n() will provide access to the global translation functions from @wordpress/i18n.

You still need to provide the current locale to <GlobalI18nProvider> somehow. You may also need to provide the current locale to <I18nProvider> somehow. That's because @wordpress/i18n doesn't give access to what the current locale is for some reason and react-i18n does. (doing #47446 instead)

In this PR I've tested it out by translating a few strings in the launch flow and I've gotten the current locale by having the server render it to a global. I previously thought I'd be able to grab the locale slug from document.documentElement.lang but unfortunately it doesn't always match the locale slug (e.g. sometimes the locale is fr and the <html lang="..."> attribute is fr_CA)

Testing instructions

Apply D52644-code to sandbox
Sandbox an unlaunched gutenboarding site
Ensure language is something other than en
Open the block editor for your test site and click the launch button in the top right
The 4 steps in the sidebar should still be translated

matticbot · 2020-11-12T04:56:04Z

Test live: https://calypso.live/?branch=try/use-global-i18n-with-react-i18n

matticbot · 2020-11-12T05:05:31Z

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D52644-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

matticbot · 2020-11-12T05:11:54Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~42 bytes removed 📉 [gzipped])

name                 parsed_size           gzip_size
entry-main                 -49 B  (-0.0%)      -23 B  (-0.0%)
entry-gutenboarding        -49 B  (-0.0%)      -19 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

p-jackson · 2020-11-12T05:30:05Z

@sirreal I think you were involved at the start of @automattic/react-i18n, could you take a look to see whether this seems like an ok approach? Also do you happen to have any insights into why @wordpress/i18n doesn't expose the current locale?

jsnajdr · 2020-11-12T09:56:45Z

This looks like an OK solution that solves a product issue. But I wish we could refactor it to something that doesn't need to introduce a new provider. The existing one should be able to work even with the default instance of I18n:

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

<I18nProvider localeData={ defaultI18n.getLocaleData() }>

To make that happen, the @wordpress/i18n library should export the defaultI18n object, not only a subset of its bound methods like __. And it should get a new getLocaleData() getter method.

do you happen to have any insights into why @wordpress/i18n doesn't expose the current locale?

I'm not sure it's guaranteed that the library always knows the locale 🙂 All it needs to work is a localeData object that maps English strings to other strings. The information about what the "other" language actually is is optional. The localeData objects have a special '' key with some metadata about the language:

{
  "": {
    "plural_forms": "nplurals=3; plural=(n==1) ? 0 : (n>=2 && n<=4) ? 1 : 2;",
    "language": "cs_CZ",
    "localeSlug":"cs"
  }
}

But I don't know if it's 100% guaranteed in practice that this special key will always be there. Without it, the library will incorrectly format plurals, because it doesn't have information about the target language's grammar and will use English rules as default. But other than that, it will work.

Once the getLocaleData() getter is exposed, the localeSlug can be accessed as

i18n.getLocaleData()[ '' ].localeSlug

or, preferrably, there can be convenience getters for the language and localeSlug values.

All these would be a valuable enhancements to @wordpress/i18n that in the end remove the need for an extra <GlobalI18nProvider>.

i18n-calypso already has these methods and adding them to @wordpress/i18n would lead to further convergence between both libraries.

yuliyan · 2020-11-12T13:13:01Z

Once the getLocaleData() getter is exposed, the localeSlug can be accessed as
i18n.getLocaleData()[ '' ].localeSlug
or, preferrably, there can be convenience getters for the language and localeSlug values.

@jsnajdr, I like the suggested approach, but want to add that language, localeSlug and localeVariant should not be considered as standardized properties. We have them being added on WP.com with the internal i18n deploy process, but it's not guaranteed that these will exist on Jed json files generated from elsewhere. For example, GlotPress' Jed files include only lang property.

p-jackson · 2020-11-12T23:13:46Z

The existing one should be able to work even with the default instance of I18n

This would be great! My original idea was something like <I18nProvider i18n={ defaultI18n }> until I realised that GB wasn't exposing the default I18n instance.

To make that happen, the @wordpress/data library should export the defaultI18n object, not only a subset of its bound methods like __.

Just to clarify @jsnajdr, did you mean the @wordpress/i18n package? Or do you think the locale data needs to be exposed via a Redux store? Personally I think just having more access to @wordpress/i18n is enough and we don't need to move things to @wordpress/data.

But I don't know if it's 100% guaranteed in practice that this special key will always be there

Thanks, good to know. We're probably relying on the locale slug too much. We use it to add ?locale=es query params to some API calls, and while that's needed in some places, for some of the APIs we're calling we should probably just add support for ?locale=user. localizeUrl() does assume that the locale will be known on the client.

language, localeSlug and localeVariant should not be considered as standardized properties. We have them being added on WP.com with the internal i18n deploy process ...

So it sounds like it's ok for our a8c specific functions (like localizeUrl()) to use these properties, but anything that we expect to work in core shouldn't refer to these properties at all.

jsnajdr · 2020-11-13T09:28:45Z

Just to clarify @jsnajdr, did you mean the @wordpress/i18n package?

Yes, I always meant @wordpress/i18n and the data package has absolutely nothing to do with this 🙂 I fixed my original comment now.

lsl · 2020-11-16T00:17:17Z

packages/react-i18n/src/index.tsx

+			_nx,
+			_x,
+			isRTL,
+			i18nLocale: localeSlug,


Can you add sprintf here too? - it doesn't make sense to leave it out

The I18n interface doesn’t include sprintf because it’s a dumb formatting utility, it doesn’t need access to any locale data. I suppose it might be convenient, but it doesn’t feel right to me to provide something with a hook that can be gotten from an import.

The @wordpress/i18n api does include sprintf though, and thats what we're trying to mirror / expose / augment here as a hook, that's why see it as worth including, adding it all into the hook makes usage more consistent and easier to explain / demonstrate.

it doesn’t feel right to me to provide something with a hook that can be gotten from an import.

Agree in general, this situation for me is more about providing a consistent experience when using l10n functions. I would even argue it isn't worth exposing by wordpress/i18n at all - its just a simple wrapper around another library - but I suspect it was included for this consistency reason to ensure everyone had all the tools in a one stop shop sort of way.

For the record, don't mind too much either way, go with your gut

import { useI18n, sprintf } from '@automattic/react-i18n' const { __, _x } = useI18n()

vs

import { useI18n } from '@automattic/react-i18n' const { __, _x, sprintf } = useI18n()

p-jackson · 2020-11-16T02:55:35Z

@lsl What do you think about these changes? I've removed <GlobalI18nProvider> and now if the <I18nProvider> component is used without a localeData prop it will default to using the global translation functions.

lsl · 2020-11-16T04:32:26Z

packages/react-i18n/src/index.tsx

 *
 * @returns The context value with bound translation functions
 */
-function makeContextValue( localeData?: LocaleData ): I18nReact {
+function makeContextValue( localeData?: LocaleData, localeSlug?: string ): I18nReact {
+	const i18nLocale = localeSlug ?? localeData?.[ '' ]?.localeSlug ?? 'en';


Neither accessing localeSlug or the en fallback work here, localeSlug is non standard and en fallback can't be assumed otherwise.

(the en fallback above is fine, I just don't think it makes sense in this package)

Indeed. The useI18n() hook guarantees that it'll always return a value for i18nLocale. However maybe that's not realistic. Perhaps we should change the return type of useI18n() to this:

export interface I18nReact { __: I18n[ '__' ]; _n: I18n[ '_n' ]; _nx: I18n[ '_nx' ]; _x: I18n[ '_x' ]; isRTL: I18n[ 'isRTL' ]; i18nLocale?: string; }

Note the ? on the i18nLocale prop. That way we'd fall back to undefined instead of 'en'.

Another option I've been thinking about is completely removing the i18nLocale prop from useI18n(). react-i18n is supposed to be something we could push upstream, and since having the locale slug available in the locale data is non-standard, it probably won't survive the review process.

We've been explicitly referring to the locale slug a lot in gutenboarding, but that's a special case because the user is logged out, and they can provide the locale in the url. That's not usually how things work. Perhaps the locale slug can be provided to a new thing, like a <LocaleProvider>, that's part of an a8c-specific package. i18n-utils I guess. That way we can depend on it for our a8c helper functions like localizeUrl() and we know we can provide it at app boot time because of the idiosyncrasies of our WordPress environment.

I don't think an optional locale slug value works - users of the prop will have to deal with that case everywhere its used, the default value needs to be decided by the consumer of react-i18n. In a lot of cases that will be english but its an assumption we can't force on everyone.

This another other option sounds really good - feels much cleaner and I like stepping away from making decisions about what a locale is.

#47446 - I like this solution to break apart the locale slug from the locale data, but we need some changes to break away from calypso-i18n #47446 (review)

cc @Automattic/i18n might want to be aware of this

I think we should go with #47446. It probably needs to merge first so this one can be updated without breaking the build.

p-jackson · 2020-11-18T08:31:53Z

As of #47446 the <I18nProvider> should no longer takes a localeSlug prop. We'll see which PR lands first!

lsl

Per #47352 (comment)

Requesting changes to block this PR until #47446 is merged.

We may be able to live with i18nLocale always being 'en' when using the default translation functions, so that we aren't blocked waiting on a decision about whether @wordpress/i18n will expose the raw locale data.

jsnajdr

Looks good 👍 Doing something about the i18nLocale being always en would be desirable, but it's up to you to decide what to do and when. Thanks for improving the react-i18n library!

jsnajdr · 2020-11-23T13:30:20Z

packages/react-i18n/src/index.tsx

 	const i18nLocale = localeData?.[ '' ]?.localeSlug ?? 'en';
+
+	if ( ! localeData ) {
+		return { __, _n, _nx, _x, isRTL, i18nLocale };


As discussed on a P2, the i18nLocale's default value en will be wrong if the upstream global i18n instance is actually localized.

A good alternative would be to not pass the i18nLocale prop in this case at all, or completely remove it from the context value.

I'm going to remove i18nLocale since currently there's currently nothing depending on it. Not ruling out that it won't eventually make it's way back in some form though.

jsnajdr · 2020-11-23T13:33:46Z

apps/editing-toolkit/editing-toolkit-plugin/editor-site-launch/src/attach-launch-sidebar.tsx

-			<LaunchContext.Provider value={ { siteId: window._currentSiteId } }>
-				<LaunchModal onClose={ closeSidebar } />
-			</LaunchContext.Provider>
+			<I18nProvider>


You don't even need to mount the provider here: the default context value, derived from undefined localeData, will be used in that case.

Good thinking, I'd somehow missed this in the midst of all these changes. I actually like this behaviour 😎

There's no way to know the correct locale slug when `localeData` is left undefined, so just removing it since there's currently no uses of it. The current locale slug might come back to react-i18n in the future once there's a standard way to get access to the current locale slug.

p-jackson self-assigned this Nov 12, 2020

p-jackson changed the title ~~GlobalI18nProvider which uses existing __ instead of creating one~~ <GlobalI18nProvider> which uses existing __ instead of creating one Nov 12, 2020

p-jackson changed the title ~~<GlobalI18nProvider> which uses existing __ instead of creating one~~ Add a provider that makes the useI18n() hook use the global translation functions from @wordpress/i18n Nov 12, 2020

p-jackson requested review from a team November 12, 2020 05:25

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 12, 2020

p-jackson added the i18n label Nov 12, 2020

p-jackson marked this pull request as ready for review November 12, 2020 05:26

sirreal requested review from jsnajdr and yuliyan November 12, 2020 07:36

p-jackson force-pushed the try/use-global-i18n-with-react-i18n branch from db7d3ed to f24a319 Compare November 13, 2020 09:34

lsl reviewed Nov 16, 2020

View reviewed changes

p-jackson force-pushed the try/use-global-i18n-with-react-i18n branch 2 times, most recently from 9122f05 to a2e1496 Compare November 16, 2020 02:04

p-jackson added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 16, 2020

lsl reviewed Nov 16, 2020

View reviewed changes

p-jackson mentioned this pull request Nov 16, 2020

Gutenboarding: strings in onboarding and launch packages should be translatable #47445

Merged

lsl mentioned this pull request Nov 17, 2020

Update/i18n utils unreverted #47492

Closed

p-jackson changed the title ~~Add a provider that makes the useI18n() hook use the global translation functions from @wordpress/i18n~~ Update provider so the useI18n() hook can use the global translation functions from @wordpress/i18n Nov 18, 2020

p-jackson force-pushed the try/use-global-i18n-with-react-i18n branch from a2e1496 to 0969d39 Compare November 18, 2020 08:33

lsl previously requested changes Nov 19, 2020

View reviewed changes

p-jackson force-pushed the try/use-global-i18n-with-react-i18n branch 2 times, most recently from 5899257 to de7947d Compare November 20, 2020 02:52

sarayourfriend changed the base branch from master to trunk November 20, 2020 16:10

p-jackson force-pushed the try/use-global-i18n-with-react-i18n branch from de7947d to e4947af Compare November 23, 2020 02:45

jsnajdr approved these changes Nov 23, 2020

View reviewed changes

yuliyan mentioned this pull request Nov 23, 2020

react-i18n: Add filter hooks #44743

Merged

p-jackson added 3 commits November 24, 2020 10:00

I18nProvider uses global __ function if localeData is left out

16fb6de

Launch sidebar uses react-i18n for some of it's translations

906f0ce

p-jackson force-pushed the try/use-global-i18n-with-react-i18n branch from e4947af to 5e1c63d Compare November 23, 2020 21:06

p-jackson mentioned this pull request Nov 23, 2020

Remove locale from react-i18n now that it is available in i18n-utils #47446

Closed

p-jackson merged commit 2c8ffc9 into trunk Nov 23, 2020

p-jackson deleted the try/use-global-i18n-with-react-i18n branch November 23, 2020 21:40

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 23, 2020

p-jackson mentioned this pull request Nov 24, 2020

Editing Toolkit: Update to 2.8.8 #47686

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update provider so the `useI18n()` hook can use the global translation functions from `@wordpress/i18n` #47352

Update provider so the `useI18n()` hook can use the global translation functions from `@wordpress/i18n` #47352

p-jackson commented Nov 12, 2020 •

edited

Loading

matticbot commented Nov 12, 2020

matticbot commented Nov 12, 2020

matticbot commented Nov 12, 2020 •

edited

Loading

p-jackson commented Nov 12, 2020

jsnajdr commented Nov 12, 2020 •

edited

Loading

yuliyan commented Nov 12, 2020 •

edited

Loading

p-jackson commented Nov 12, 2020

jsnajdr commented Nov 13, 2020

lsl Nov 16, 2020

p-jackson Nov 16, 2020

lsl Nov 16, 2020

lsl Nov 16, 2020

p-jackson commented Nov 16, 2020

lsl Nov 16, 2020

lsl Nov 16, 2020

p-jackson Nov 16, 2020 •

edited

Loading

lsl Nov 16, 2020

lsl Nov 16, 2020

p-jackson Nov 18, 2020

p-jackson commented Nov 18, 2020

lsl left a comment

jsnajdr left a comment

jsnajdr Nov 23, 2020

p-jackson Nov 23, 2020

jsnajdr Nov 23, 2020

p-jackson Nov 23, 2020

Update provider so the useI18n() hook can use the global translation functions from @wordpress/i18n #47352

Update provider so the useI18n() hook can use the global translation functions from @wordpress/i18n #47352

Conversation

p-jackson commented Nov 12, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Nov 12, 2020

matticbot commented Nov 12, 2020

matticbot commented Nov 12, 2020 • edited Loading

p-jackson commented Nov 12, 2020

jsnajdr commented Nov 12, 2020 • edited Loading

yuliyan commented Nov 12, 2020 • edited Loading

p-jackson commented Nov 12, 2020

jsnajdr commented Nov 13, 2020

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

p-jackson commented Nov 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-jackson Nov 16, 2020 • edited Loading

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

p-jackson commented Nov 18, 2020

lsl left a comment

Choose a reason for hiding this comment

jsnajdr left a 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

Update provider so the `useI18n()` hook can use the global translation functions from `@wordpress/i18n` #47352

Update provider so the `useI18n()` hook can use the global translation functions from `@wordpress/i18n` #47352

p-jackson commented Nov 12, 2020 •

edited

Loading

matticbot commented Nov 12, 2020 •

edited

Loading

jsnajdr commented Nov 12, 2020 •

edited

Loading

yuliyan commented Nov 12, 2020 •

edited

Loading

p-jackson Nov 16, 2020 •

edited

Loading