Make i18n functions filter their return values

[leewillis77]

Description

In WordPress, every single string that is passed to __() & friends in PHP can be filtered and overridden. Currently though, strings passed through the Javascript i18n equivalent functions aren't filterable and cannot easily be dynamically overridden.

This affects the ability for people to easily override, or change strings on their site that exist in Javascript features. As adoption of the Javascript infrastructure increases (Gutenburg, WooCommerce admin, 3rd party blocks etc.) this becomes more important.

This was originally raised in #12516, with a PR (#12517) by @swissspidy, however it wasn't progressed mainly due to perceived lack of a use-case.

I'm the author of the Say What? plugin (https://wordpress.org/plugins/say-what/) which uses the PHP gettext filters (__() and friends) to do its job. Currently there's no way (that I can tell) that it can do the same job on JS strings, and I'm already seeing users reach out as they can't change some strings that end up being generated via the JS infrastructure so aren't currently filterable.

With the changes in this PR, equivalent filters are available in JS as they are in PHP, and I am able to replicate the features in my plugin for JS strings entirely.

The code is based on @swissspidy's original PR but I had to re-work some to get it to work against the code as-is today. JS development is not my strength, so I'm sure there are improvements others can suggest (particularly the String() calls seem "icky" to me, but I'm unsure on the "right" way to address the type matching).

Happy to update the PR based on feedback, or provide any other information useful for progressing the changes.

How has this been tested?

Manual testing only to verify that the added filters allow strings to be overriden.

Types of changes

This PR introduces four new filters:

• applyFilters( 'i18n.gettext', translation, text, domain )
• applyFilters( 'i18n.gettext_with_context', translation, text, context, domain )
• applyFilters( 'i18n.ngettext', translation, single, plural, number, domain )
• applyFilters( 'i18n.ngettext_with_context', translation, single, plural, number, context, domain )

[swissspidy]

Suggested change

	"@wordpress/hooks": "^2.11.0",
	"@wordpress/hooks": "file:../hooks",

[leewillis77]

Updated. Thanks!

[swissspidy]

Thanks for picking this up again.

Note that there are still very legitimate performance concerns with this approach:

#12517 (comment)
#12517 (comment)

A more performant way to let devs override strings might be to use setLocaleData() or similar

[leewillis77]

@swissspidy Thanks for your feedback (& original PR!)

From my reading of the comments (particularly #12517 (comment)) on the original thread the crux seemed to be that this might be a performance issue, and therefore was little appetite to progress the PR without a solid use case.

I'm hoping now that we have a use-case we can explore those issues.

There seems to be a few suggestions:

1. Benchmark to see if this is actually an issue
2. If so, potential to improve this in the hooks package itself (Make gettext functions filterable #12517 (comment))
3. Optimize in the i18n package depending on whether hooks have been added (Make gettext functions filterable #12517 (comment))

Unfortunately, I'm not an expert in the Javascript ecosystem, so not sure how we'd go about benchmarking (if indeed it's necessary), but happy to help where I can. I'll try and jump into the next core-js discussion on slack to gather opinions on benchmarking / optimisation options.

[jsnajdr]

The createI18n function shouldn't always use the global applyFilters function. It should allow for supplying a custom instance of the Hooks object returned by createHooks.

The hooks and i18n packages have a similar structure:

• there is a createI18n or createHooks function that creates a I18n or Hooks object that has methods like __ or applyFilters
• the program that uses these libraries can create multiple instances of these objects that are fully independent from each other and don't share any data. Imagine a Node.js server that returns server-generated pages, serving multiple users, and handling the requests in parallel. Each request has its own, isolated context.
• both libraries create a "default" instance of their respective objects, and exports their methods as functions. That's what __ and applyFilters are -- methods of one specific object instance that are used as functions.

To keep the separation between default singletons and custom instances, createI18n should take a new hooks parameter:

createI18n( initialData, initialDomain, hooks )

use this hooks parameter to get to the right applyFilters:

return String( hooks.applyFilters( ... ) );

and the default-i18n.js module should configure it with the default hooks:

import { applyFilters } from '@wordpress/hooks';

// if the hooks package exported the `Hooks` object itself, we would pass it as is and
// wouldn't have to create a 'fake' one with `{ applyFilters }`
const i18n = createI18n( null, null, { applyFilters } );

[leewillis77]

@jsnajdr I've updated the PR to follow this approach. The hooks arg is optional, and no filters will be called if it is not provided. default-i18n provides a copy of applyFilters from @wordpress/hooks when creating the default instance. Feedback welcome.

[jsnajdr]

Core recently added also domain-specific filters named i18n.gettext_${ domain }:

https://core.trac.wordpress.org/ticket/49518

Can we add them here, too, to keep the APIs as similar as possible?

[leewillis77]

@jsnajdr I'm happy to update to include that, however there is a slight difference here in implementations. In core "domain" is defaulted to "default" if not provided, but the JS methods don't do that (leaving "domain" as undefined). Given your comments previously about re-use of the resulting object I'm not sure whether we should:
a) update the __() etc. method signatures to set domain to "default" if not provided,
b) call a "default" filter if typeof domain === 'undefined'

Any thoughts?

[jsnajdr]

@leewillis77 The b) option looks better to me as it achieves the goal without changing the library too much. But I'm not really sure. I noticed the new gettext_${ domain } hooks when comparing the hooks in this PRs to what's already in Core/PHP. Anyway, adding these extra hooks is not a blocker and can be done at any later time.

[jsnajdr]

FYI for @yuliyan: this is related to your recent work on react-i18n hooks and could be interesting for you.

[swissspidy]

@yuliyan Are you actively working on react-i18n stuff? If not, I have some WIP code that I want to open a PR for (demo: https://twitter.com/swissspidy/status/1343603284460924928)

[jsnajdr]

@swissspidy In your client-side locale switching demo, did you use the react-i18n library or did you write something similar in spirit from scratch?

The react-i18n library currently lives in the Calypso repo, which acted like a kind of an incubator for it, and is now ready to be transferred to the Gutenberg repo. First week in January looks like a good time to do that 🙂

[swissspidy]

@swissspidy In your client-side locale switching demo, did you use the react-i18n library or did you write something similar in spirit from scratch?

The react-i18n library currently lives in the Calypso repo, which acted like a kind of an incubator for it, and is now ready to be transferred to the Gutenberg repo. First week in January looks like a good time to do that 🙂

I was not aware of that package, so yeah I built something similar from scratch. It not simply bind to the existing i18n functions though.

Just pushed my very early code here: #27973. PTAL! :)

[leewillis77]

This is the outcome of the existing post-editor performance test against the current PR:

This obviously will be triggering the applyFilters calls on all translatable strings, although there is nothing attached to those hooks in the current tests so I guess it's a good comparison of the common case. The biggest change would appear to be the load time, but I'm not familiar enough with the accuracy of the tests to know if that's an issue, or within the bounds of expected variance?

>> post-editor

┌──────────────────┬──────────────────────────────────────────┬─────────────┐
│     (index)      │ 0514d0c34e5ac8762777a2d7ff762d346e0bfc85 │   master    │
├──────────────────┼──────────────────────────────────────────┼─────────────┤
│       load       │                '6580 ms'                 │  '6390 ms'  │
│       type       │                '39.23 ms'                │ '38.47 ms'  │
│     minType      │                '31.17 ms'                │ '31.88 ms'  │
│     maxType      │                '86.1 ms'                 │ '89.81 ms'  │
│      focus       │                '91.27 ms'                │ '90.63 ms'  │
│     minFocus     │                '78.6 ms'                 │ '80.12 ms'  │
│     maxFocus     │               '125.01 ms'                │ '126.57 ms' │
│   inserterOpen   │               '376.11 ms'                │ '381.5 ms'  │
│ minInserterOpen  │                '345.8 ms'                │ '355.16 ms' │
│ maxInserterOpen  │               '509.82 ms'                │ '527.71 ms' │
│  inserterHover   │                '34.21 ms'                │ '33.56 ms'  │
│ minInserterHover │                '30.3 ms'                 │ '29.85 ms'  │
│ maxInserterHover │                '45.15 ms'                │ '43.68 ms'  │
└──────────────────┴──────────────────────────────────────────┴─────────────┘

I'm going to duplicate the post-editor test and attach some simple filters to see what the timings of that look like versus this as well.

[leewillis77]

Test failures don't appear to relate to this PR since they're also failing on other PRs, e.g. https://github.com/WordPress/gutenberg/actions/runs/511761863

[adamsilverstein]

@jsnajdr Is anything else needed here, or can we go ahead and merge this?

[yuliyan]

Converting the translation to always be of a string type seems a bit restrictive. Is there a reason not to allow it to be filtered into any type of value?

[swissspidy]

The type definitions / JSDoc say that it returns a string, so it makes sense to uphold that promise, no?

[yuliyan]

Would it be worth removing the type conversion and updating the JSDoc? I think it will make the filters a lot more flexible, especially in a React context, where it might not be uncommon to need to return a React component directly as a result of the gettext call.

[jsnajdr]

When used in a React component:

<span>{ __( 'Hello' ) }</span>

The return value of __() can be a generic ReactNode, which includes many possible value types, string being one of them.

It would be nice to keep this flexibility -- there are creative use cases that benefit from it.

The return type of hooks.applyFilters is unknown. If we want to coerce that into a string, we can use a type annotation:

translation = /** @type {string} */ hooks.applyFilters();

That affects only the type checker, and doesn't do runtime conversions like String() does.

[leewillis77]

I'm more than happy to make this change (The String() calls always made me feel a little uneasy!) I've tried the annotation approach required here, but can't seem to make it compile, and unfortunately I don't know enough about the syntax to coax it into submission.

packages/i18n/src/create-i18n.js:179:3 - error TS2322: Type 'unknown' is not assignable to type 'string'.

179  	translation = /** @type {string} */ hooks.applyFilters(
     	~~~~~~~~~~~

[leewillis77]

@jsnajdr It looks like applyFilters returns unknown rather than any which means (I think) that we can't just type hint it?

[jsnajdr]

Interesting. The unknown type should be assignable to anything else, without constraints. I though that value as string is exactly the same thing as /** @type {string} */ value, but TypeScript playground reports error only for the second (jsdoc) one:

@sirreal Do you have any insight about what's going on? Maybe there is some TS option that affects this?

[jsnajdr]

@leewillis77 I found that this workaround works:

translation = /** @type {string} */ ( /** @type {*} */ applyFilters( ... ) );

[leewillis77]

@jsnajdr Thanks. PR updated.

[sirreal]

EDIT This is a response to a few previous comments the thread and focuses on the types. It does not take into account the other context and is not a request for changes.

Interesting. The unknown type should be assignable to anything else, without constraints.

@sirreal Do you have any insight about what's going on? Maybe there is some TS option that affects this?

Anything can be assigned to the unknown type, but unknown does not satisfy any other types. any is the type that satisfies all other type (and anything can be assigned to any.

You can read more about unknown in its announcement.

TypeScript 3.0 introduces a new top type unknown. unknown is the type-safe counterpart of any. Anything is assignable to unknown, but unknown isn’t assignable to anything but itself and any without a type assertion or a control flow based narrowing. Likewise, no operations are permitted on an unknown without first asserting or narrowing to a more specific type.

When we really don't have type information, especially at the boundaries of our typed application like a REST response or receiving data across untyped APIs like here, unknown makes that very clear and forces you to narrow with guards or type assertions. Before unknown, there was only any which was completely unsafe because you can put anything in and do anything with it.

The error is expected and the fix is what TypeScript recommends (this is a type assertion written in JSDoc). First, we assert the unknown is any, then we can narrow it to the type of our choosing because any:

translation = /** @type {string} */ ( /** @type {*} */ applyFilters( ... ) );

A (safer) alternative would be to use a type guard, but has a runtime cost:

const filteredTranslation = applyFilters( /* … */ );
return typeof filteredTranslation === 'string' ? filteredTranslation : translation; // we know it's a string

[jsnajdr]

Is anything else needed here, or can we go ahead and merge this?

@adamsilverstein There is a good suggestion by @yuliyan in #27966 (comment) to remove the String() conversions after calling hooks.applyFilters. @leewillis77 would you agree to remove them and possibly replace with /** @type {string} */ annotations?

If this issue is resolved, I think we can merge the PR and celebrate 🎉

[jsnajdr]

Thanks @leewillis77 for working on this! I'm going to merge the PR now 👍

[gziolo]

@youknowriad – do you think it's something we want to keep? I guess it's fine to have it to prevent regressions. It might be good to extract some common logic though. It looks like performance tests take over 30 minutes to run now.

[gziolo]

Great job everyone, it's so nice to see it landed finally. We could close the following issues: #12516, #14833, #18743. There could be more of it but I didn't immediately find references.

We still miss good documentation explaining how to change a given translation on the page. It deserves its own page in this folder:
https://github.com/WordPress/gutenberg/tree/master/docs/designers-developers/developers/filters

[gziolo]

I have just realized that it will make it to WordPress 5.7. We will need a dev note as well to let everyone know, @noisysocks can you add it to the list?

@leewillis77, it looks like it's your first commit to Gutenberg. Congrats 🎉

[leewillis77]

@gziolo I'll work on a docs article today, and raise a separate PR (?) to add it.

[gziolo]

That sounds like an excellent plan ❤️

[leewillis77]

@gziolo @noisysocks

PR #28553 raised with markdown docs for the new filters.