i18n-calypso: add useTranslate hook #31043

jsnajdr · 2019-02-26T16:55:23Z

The second React hook in Calypso 🎉 (after #31035)

useTranslate is a hook version of the localize HOC that provides the translate function and the localeSlug string, nothing else.

Adding also first use in blocks/post-share/no-connections-notice. See #31022 for instructions where to find it and how to test it.

Still work in progress, need to add tests (I have no idea how to test hooks at this moment) and documentation.

matticbot · 2019-02-26T16:55:27Z

Test live: https://calypso.live/?branch=add/use-translate-hook

jsnajdr · 2019-02-27T09:36:37Z

This is now ready for review. I did some optimizations to prevent creating unneeded objects on every hook call. Added section about the hook to README. And added a basic unit test.

I'd also love to test the reactive rerenders of useTranslate on locale change, but don't know how to do that. The React hooks unit testing story is still in early phases. Enzyme doesn't have support for hooks. Howto articles on the web are usually very long and complicated 😄 I gave up for now.

ockham · 2019-02-27T11:17:53Z

Sidenote, could this be relevant for Gutenberg? See WordPress/gutenberg#9846 specifically.

sgomes

LGTM, great seeing hooks making their way into the codebase :) I left some comments for you to consider.

sgomes · 2019-02-27T18:17:07Z

packages/i18n-calypso/src/use-translate.js

+			const onChange = () => setLocaleSlug( getLocaleSlug() );
+			i18n.on( 'change', onChange );
+			return () => i18n.off( 'change', onChange );
+		}, []);


The empty array is an important detail here to make sure that the effect is only applied on mount, and not on every update. Good work 👍

sgomes · 2019-02-27T18:21:52Z

packages/i18n-calypso/src/use-translate.js

+			return () => i18n.off( 'change', onChange );
+		}, []);
+
+		return [ translate, localeSlug ];


I'm not sure I'd use an array here; the two properties aren't as closely-related as the [ property, setProperty ] pair returned by useState, and they likely won't need to be renamed either. I'd probably return an object instead.

Still, this is really a matter of personal taste here, so happy either way.

Another option is to just return translate:

const translate = useTranslate();

Components never really use anything else. The localeSlug needs to be part of state, because the translate function is always the same and something needs to change to trigger a rerender. That will be especially true if we switch to React Context instead of one i18n listener per each mounted localized component.

I returned array mainly because it's an existing convention for several hooks. But thinking about it a little more, it doesn't look like a good fit for this particular hook.

+1 to just returning translate

Would it be possible to make the language slug available as translate.localeSlug?

sgomes · 2019-02-27T18:26:01Z

packages/i18n-calypso/test/use-translate.js

+		renderer.render( <Label /> );
+
+		// check that it's translated
+		expect( renderer.getRenderOutput() ).toBe( 'háček (cs)' );


Mocking i18n might allow you to test reactivity for this hook. Take a look at the react-helpers.js test in https://github.com/Automattic/wp-calypso/pull/31081/files for a similar situation where I mocked a browser API.

sgomes · 2019-02-27T18:30:43Z

packages/i18n-calypso/src/use-translate.js

+		const [ localeSlug, setLocaleSlug ] = React.useState( getLocaleSlug );
+
+		React.useEffect(() => {
+			const onChange = () => setLocaleSlug( getLocaleSlug() );


I ran into some confusing issues in my own PR when dealing with subscriptions for multiple instances of the same component. This code looks good, but I'd suggest testing reactivity when you have multiple instances of the same component, and ensure they all get updated correctly.

blowery · 2019-02-28T17:13:35Z

packages/i18n-calypso/src/use-translate.js

+	const getLocaleSlug = i18n.getLocaleSlug.bind( i18n );
+
+	return function useTranslate() {
+		const [ localeSlug, setLocaleSlug ] = React.useState( getLocaleSlug );


See https://reactjs.org/docs/hooks-reference.html#lazy-initial-state for what passing a function to useState means. TL;DR: it calls the function to generate the first state, only calling it on the first render

Yes, that's exactly what we need here, isn't it? The first render will initialize the state value, and all further changes come from the change listener.

yup! I just found it surprising, as I didn't know that useState could take an initializer function. Sorry I wasn't more clear.

my first comment should have read: "A note to anyone else reviewing this PR, see ..."

Here's a great blog post that describes many possible gotchas: https://overreacted.io/making-setinterval-declarative-with-react-hooks/

One source of subtle bugs is capturing a stale value inside a closure:

const [ count, setCount ] = useState( 0 ); useEffect( () => { setTimeout( () => setCount( count + 1 ), 1000 ); }, [] );

Because of the [] argument, useEffect will call the effect function only once, on mount, and the count value will remain bound to 0, even if the state changed in the meantime.

It's interesting that the setCount function doesn't have this problem and can be saved without problems. It's assumed to be the same for every call. Ref objects returned from useRef() can also be saved. I don't know what exactly the rules and guarantees are -- which values are safe and which are not?

jsnajdr · 2019-02-28T17:37:08Z

I modified the hook to just return translate function, and to expose a translate.localeSlug (as a getter defined with Object.defineProperty) as @akirk suggested.

The withLocalizedMoment should get the same update if we agree it's the way to go.

As a next step, I'll look into @sgomes' suggestions on reactivity and its testing.

Use lazy creation of the initial state, create the change handler inside the `useEffect` callback. Both changes prevent creation of unneeded function objects on every hook call.

…ose translate.localeSlug property

jsnajdr · 2019-03-04T16:27:29Z

I added some unit tests that test reactive rerenders when the i18n object emits a change event. Thanks @sgomes for teaching me how to do it in #31081.

There was one subtle bug that I had to fix: i18n.setLocale can be used not only to switch to another locale, but also to update translations for the current locale. Our community translator does that, for example.

Because the localeSlug state bit stayed the same after such update, the components using useTranslate were not rerendered. There was no state update to trigger that and this.setState( {} ) no longer does that with hooks.

The solution is to create a new instance of the bound translate function on each change event.

We should eventually switch to React Context that creates just one instance of the translate function on each update and passes it down. Currently, we maintain 100+ event listeners instead of one, and now will also create 100+ function instances. Fortunately, JS engine authorities say that bound functions are extremely cheap to create and store.

jsnajdr · 2019-03-05T13:39:18Z

Sidenote, could this be relevant for Gutenberg? See WordPress/gutenberg#9846 specifically.

@ockham For a solution for the component interpolation problem specifically? Or for something else?

This PR takes the i18n-calypso API with no modifications at all and just adds a new React helper.

I'd like to learn more about the differences between @wordpress/i18n and i18n-calypso and ways how to reconcile them into one system, but at this moment, I know very little about that.

ockham · 2019-03-08T09:19:13Z

@ockham For a solution for the component interpolation problem specifically?

Yeah, for that. Sorry, I should've been clearer.

This PR takes the i18n-calypso API with no modifications at all and just adds a new React helper.

Yeah, it was mostly the React helper I thought might be a good fit for Gutenberg.

I'd like to learn more about the differences between @wordpress/i18n and i18n-calypso and ways how to reconcile them into one system, but at this moment, I know very little about that.

Yep, let's discuss that some time (maybe after we've finished moving the JP blocks code to the JP repo 😅 )

jsnajdr added Framework [Status] In Progress i18n labels Feb 26, 2019

jsnajdr self-assigned this Feb 26, 2019

jsnajdr requested review from ockham, akirk, sgomes, sirreal and a team February 26, 2019 16:55

jsnajdr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 27, 2019

sgomes approved these changes Feb 27, 2019

View reviewed changes

blowery reviewed Feb 28, 2019

View reviewed changes

jsnajdr mentioned this pull request Mar 4, 2019

Add hooks and HOCs to lib/viewport #31081

Merged

jsnajdr added 8 commits March 4, 2019 16:50

i18n-calypso: add useTranslate React hook

54a6bf0

post-share/no-connection-notice: use the useTranslate hook

4357550

Optimize the useTranslate hook a bit

a9da17c

Use lazy creation of the initial state, create the change handler inside the `useEffect` callback. Both changes prevent creation of unneeded function objects on every hook call.

Document the useTranslate hook

b996734

Add unit test for the useTranslate hook

a0df080

Returns just the translate function from the hook (not array) and exp…

9049fd2

…ose translate.localeSlug property

Use the ReactDOM rendered and JSDOM in tests

a184a19

Add unit test for useTranslate reactive rerender

571c52d

jsnajdr force-pushed the add/use-translate-hook branch from ff89b42 to e1e4a93 Compare March 4, 2019 15:50

Add test for reactivity when locale does not change, fix implementation

2cc873d

jsnajdr force-pushed the add/use-translate-hook branch from e1e4a93 to 2cc873d Compare March 4, 2019 16:25

jsnajdr merged commit ad3e868 into master Mar 5, 2019

jsnajdr deleted the add/use-translate-hook branch March 5, 2019 13: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 Mar 5, 2019

dmsnell mentioned this pull request Apr 11, 2019

Activity Log: Fix data layer on credentials save error #32180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n-calypso: add useTranslate hook #31043

i18n-calypso: add useTranslate hook #31043

jsnajdr commented Feb 26, 2019

matticbot commented Feb 26, 2019

jsnajdr commented Feb 27, 2019

ockham commented Feb 27, 2019

sgomes left a comment

sgomes Feb 27, 2019

sgomes Feb 27, 2019

jsnajdr Feb 28, 2019

blowery Feb 28, 2019

akirk Feb 28, 2019

sgomes Feb 27, 2019

sgomes Feb 27, 2019 •

edited

Loading

blowery Feb 28, 2019 •

edited

Loading

jsnajdr Feb 28, 2019

blowery Feb 28, 2019

blowery Feb 28, 2019

jsnajdr Feb 28, 2019

jsnajdr commented Feb 28, 2019

jsnajdr commented Mar 4, 2019

jsnajdr commented Mar 5, 2019

ockham commented Mar 8, 2019

i18n-calypso: add useTranslate hook #31043

i18n-calypso: add useTranslate hook #31043

Conversation

jsnajdr commented Feb 26, 2019

matticbot commented Feb 26, 2019

jsnajdr commented Feb 27, 2019

ockham commented Feb 27, 2019

sgomes 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgomes Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

blowery Feb 28, 2019 • 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

Choose a reason for hiding this comment

jsnajdr commented Feb 28, 2019

jsnajdr commented Mar 4, 2019

jsnajdr commented Mar 5, 2019

ockham commented Mar 8, 2019

sgomes Feb 27, 2019 •

edited

Loading

blowery Feb 28, 2019 •

edited

Loading