types: Uses textOnly for translations

[scinos]

Background

We have a few TS files that do translate(...).toString(). The result of translate() was declared as React.ReactNode in #58938. This type includes undefined | null, therefore calling .toString() in a value that is potentially undefined or null is not safe.

Changes proposed in this Pull Request

#58938 added the option {textOnly:true} to force the return type to be string, making calls to .toString() valid. But of course, in that case, there is no point in calling .toString() anymore, as the result of translate() is already a string.

This should fix all these type errors:

[github-actions]

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-22308
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-22308&env=jetpack

[matticbot]

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

Sections (~24 bytes removed 📉 [gzipped])

name                    parsed_size           gzip_size
jetpack-cloud-settings       -429 B  (-0.2%)      -24 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

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.

[scinos]

I'm not convinced this is the right solution:

• For starters, {textOnly:true} is only a "type mirage". There is no actual implementation for that behaviour (or at least I haven't found it). We are using it only to restrict the return type of translate() to be a string.
• Then, my change is potentially changing the behaviour of the code. Before we were calling .toString() and now we are not. Depending on the type returned by translate() this may or may not be an actual change.

Looking at the implementation of translate(), I think it can't possibly return null or undefined. I'm not sure if those types were accidentally introduced in #58938 with the migration to React.ReactNode. If that is the case, maybe we can redefine translate() to return Except<React.ReactNode, null | undefined>.

That will remove the burden from consumers to check for null | undefined, maybe remove the need to use textOnly:true in some cases, and make the original translate().toString() a valid call.

[yuliyan]

Tested, LGTM.

I saw @scinos comment afterwards.

[yuliyan]

@scinos,

The translate call is using tannin.dcnpgettext internally, which should always return a string (either the translation, or the same string it was called with, in case of a missing translation). Before returning, the translate function is additionally applying interpolateComponents and translateHooks.

The former will transform the return value to a JSX.Element when passed components translate option, and the latter can possibly have a translate hook that would transform the value into anything, including null and undefined.

Despite not having any references to { textOnly: true } in i18n-calypso itself, it's actually being used as a flag within the community translator to prevent the hook from wrapping the translation in a React component. Therefore, if interpolateComponents or any other translate hook has not already transformed the translation into anything other than string, the flag will cause the Community Translator hook to bail out early and preserve it.

[jsnajdr]

I think this is not a good solution and have some ideas how to fix this problem much better. I'll describe them later, now I'm just giving 👎 to prevent merging this PR too soon.

[jsnajdr]

In this file, the translated strings should be returned as ReactNodes, without textOnly. They eventually end up as <FormInputValidation text> props, rendered as HTML markup.

It's the ValidationError type that should be fixed. message is not a string, but a ReactNode. That basically means that the message can use HTML formatting, e.g.:

Use forward slashes, <code>/</code>, in path.

[scinos]

Fixed

[jsnajdr]

Here the items should be ReactNodes (i.e., HTML text) too: they are rendered as

<ul>{ items.map( text => <li>{ text }</li> ) }</ul>

[scinos]

Fixed

[jsnajdr]

As @yuliyan writes, translate mostly returns strings, and there are only two ways how it can return React markup:

• translate is called with the components option and component interpolation is done. We could add an additional type guard that says that translate( text, { components } ) always returns ReactNode, never a string. And that translate( text, { components, textOnly } ) are mutually exclusive.
• one of the translation hooks is active (we have two), and it wraps translations into markup like <Translatable>hello</Translatable> which highlights an unfinished translation in red and opens a dialog on click. Such a hook must ignore the textOnly: true translations because otherwise markup like <img alt={ translate( 'Hello' ) }/> would render [object Object], the stringified element object.[*]

[*] Actually TIL that the two translate hooks in Calypso do an additional trick that would render Hello anyway: they put a custom .toString method on the returned element. But component interpolation doesn't have that defense and

<img alt={ translate( '{{b}}Hello{{/b}}', { components: { b: <b/> } } ) } />

would really render [object Object].

Now, could there be a better return type than ReactNode? The translate function never really returns undefined or null, there is always some text. Turns out there is a ReactChild type that is exactly what we want:

type ReactChild = ReactElement | ReactText;

and ReactNode extends ReactChild with stuff that we mostly don't want:

type ReactNode = ReactChild | ReactFragment | ReactPortal | boolean | null | undefined

Here ReactFragment is basically an iterable object, letting me do <div>{ [].map() }</div> and ReactPortal is an extension of ReactElement whose purpose I don't understand.

I tried to change translate return type to ReactChild in #59095. And its TS error bilance is +45-96, a good result.

It will also fix situations where a ReactNode is a required attribute:

MyButton.propTypes = { text: PropTypes.node.isRequired };
const text: ReactNode = 'hello';
<MyButton text={ text } />;
// bang, ReactNode can be undefined which conflicts with the `.isRequired` bit

or

type Props = { text? ReactNode };
type ReqProps = Required<Props>;
const text: ReactNode = 'hello';
props: ReqProps = { text };
// bang, a TS error again

[scinos]

I tried to change translate return type to ReactChild in #59095. And its TS error bilance is +45-96, a good result.

So we can close this in favour of #59095, right?

[jsnajdr]

So we can close this in favour of #59095, right?

No, this one is quite independent from #59095, and still does something useful: removes the unneeded .toString() calls and fixes up the types from string to ReactNode. I'd just wait for #59095 to ship and then update ReactNode to ReactChild.