Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EXPERIMENTAL] react-i18n: client-side locale switching #27973

Closed
wants to merge 1 commit into from

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented Jan 4, 2021

Description

Explores possibilities of client-side locale switching using a new react-i18n package containing a React context provider.

As discussed in #19210

After the fact I learned that this is similar to what Automattic has been working on here.

How has this been tested?

There's some dummy dropdown in there at the moment for easier testing.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

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
Copy link
Member

sirreal commented Jan 20, 2021

There's an issue touches on this: #19210

@swissspidy
Copy link
Member Author

Yes, correct, this PR originated from the discussion on that issue. Apologies for not linking to it in the description.

return formats.filter( ( format ) =>
includes( supportedFormats, format.id )
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Using useMemo would be beneficial here, avoiding translating, sorting and filtering the array over and over on each render:

const formats = useMemo( () => {
  return [ ... ].sort( ... );
}, [ __ ] );

const filteredFormats = useMemo( () => {
  return formats.filter( ... );
}, [ formats, supportedFormats ] );

return filteredFormats;

@swissspidy
Copy link
Member Author

In a next step I'll factor out the REST API integration (effect to load new translations upon locale changing) out of the react-i18n package, perhaps into the new user preferences modal in the works (#28329).

I assume this can be easily moved to a second follow-up PR too.

@swissspidy swissspidy added [Package] i18n /packages/i18n Internationalization (i18n) Issues or PRs related to internationalization efforts [Type] Enhancement A suggestion for improvement. [Type] New API New API to be used by plugin developers or package users. labels Jan 21, 2021
localeMap.get( locale ).setLocaleData( data );
setLocaleMap(
new Map( localeMap.set( locale, localeMap.get( locale ) ) )
);
Copy link
Member

Choose a reason for hiding this comment

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

This could be just setLocaleMap( new Map( localeMap ) ). The set( get() ) bit doesn't do anything. We just need to update state with a new copy of the map that's not === to the previous one.


return {
locale,
__,
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 very useful to bind the __ functions to create unique instances on every context change: return { __: __.bind() }. Without this, after setLocaleData() the i18n object is still the same and therefore the __ function is still the same. Even though the translation data have changed and components should be updated.

If the __ value doesn't change, usages like this will fail to rerender and update:

const Heading = React.memo( ( { __ } ) => {
  return <h1>{ __( 'Warning' ) }</h1>;
} );

const Box = ( text ) => {
  const { __ } = useTranslate();
  return (
    <div>
      <Heading __={ __ } />
      <p>{ __( 'Hello' ) }</p>
    </div>
  );
}

A component that uses React.memo or React.PureComponent will rerender only when props change, and for <Heading /> and setLocaleData() they don't.

This scenario will happen any time there is a withTranslate higher-order component, which passes __ down as a prop, and a Redux-connect-ed component, which by default behaves as pure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good call!

@jsnajdr
Copy link
Member

jsnajdr commented Jan 21, 2021

This could be factored out into three independent parts:

The react-i18n library with a context provider and consumer hook. It takes the @wordpress/i18n I18n object as a provider parameter:

const i18n = createI18n();
<Provider i18n={ i18n } />

and makes the localization data available to the entire React tree, with reactive updates.

Updates are made when the i18n instance is swapped for another one, and also when i18n.setLocaleData() is called on an existing instance. To notify the React bindings about the setLocaleData() changes, the i18n instance would need a new subscribe method that fires an event every time setLocaleData() is changed.

This react-i18n binding is pure consumer, only reading from i18n and never changing it. The "changing it" part depends a lot on the actual app, so it's better kept separate. The useTranslate hook wouldn't provide access to setLocale and setLocaleData.

Such a react-i18n binding is almost 100% compatible with what we already have in Calypso (https://github.com/Automattic/wp-calypso/tree/trunk/packages/react-i18n) and are about to donate to the Gutenberg repo. I expect reconciling these two implementations to be very easy.

The only wrinkle is the fact that the Calypso react-i18n provider takes localeData as a prop, not the i18n instance. @sirreal do you remember why we did this? Because the @wordpress/i18n library didn't provide the getLocaleData and subscribe methods?

The other little difference is the naming of the hook: useTranslate vs useI18n.

The second part is the map of locale data for different languages. This can be kept separately from the react-i18n context, and the "selected" i18n instance can be passed to the provider like <Provider i18n={ localeMap.current() } />.

The third part would be the code that async-loads the locale data from REST API or any other source.

@swissspidy
Copy link
Member Author

Makes sense to me!

I think I could easily update this PR accordingly.

The second part is the map of locale data for different languages. This can be kept separately from the react-i18n context, and the "selected" i18n instance can be passed to the provider like <Provider i18n={ localeMap.current() } />.

I see! So localeMap would live somewhere else, and then e.g. the @wordpress/edit-post package (where I currently use the LocaleProvider) would just pass the map's current i18n instance to the LocaleProvider.

The third part would be the code that async-loads the locale data from REST API or any other source.

Are you imagining this to also hold the localeMap and allow switching locales? Or just the async-loading part?

@sirreal
Copy link
Member

sirreal commented Jan 21, 2021

The only wrinkle is the fact that the Calypso react-i18n provider takes localeData as a prop, not the i18n instance. @sirreal do you remember why we did this? Because the @wordpress/i18n library didn't provide the getLocaleData and subscribe methods?

We worked on react-i18n before the i18n package exposed instances. Yes, I think this was implemented as it is to work around those limitations.


I'd also favor the idea of keeping react-i18n very small and focused on providing react bindings for translation, and separating application-specific code into another layer like was suggested here.

@jsnajdr
Copy link
Member

jsnajdr commented Jan 22, 2021

Are you imagining this to also hold the localeMap and allow switching locales? Or just the async-loading part?

I don't know at this moment, but I think there's a good way how to figure it out: @sirreal wrote a very similar language switching code for the WordPress.com Gutenboarding project. That's a React app that runs almost completely outside Gutenberg and WordPress, and it just uses the @wordpress/* libraries to implement the UI.

If we compare the two implementations, we could get some clues about which parts are generic and reusable and which are app-specific. I'll return to this work on Monday 🙂

@sirreal
Copy link
Member

sirreal commented Jan 22, 2021

For reference, the work is open source here:

  • This is the application specific bit, CalypsoI18nProvider. This renders the provider from react-i18n which is very generic in a way that works with our application. In this case, it mostly links into our pre-existing i18n system.
  • The very generic react-i18n implementation we worked on is here. There are likely a few improvements to make since @wordpress/i18n has evolved a bit.

@jsnajdr
Copy link
Member

jsnajdr commented Jan 25, 2021

In #28465 I'm proposing a react-i18n library that offers a consumer-only React bindings for @wordpress/i18n. Based on this PR and the discussion here.

If this PR is rebased on top of that one, it should only contain the code that maintains a map of I18n instances, loads the translation files from network, and the code that actually uses the reactive translations in the editor and edit-post packages.

Base automatically changed from master to trunk March 1, 2021 15:45
@swissspidy swissspidy closed this Jan 26, 2023
@swissspidy swissspidy deleted the try/swissspidy-react-i18n branch January 26, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] i18n /packages/i18n [Type] Enhancement A suggestion for improvement. [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants