-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make i18n functions filter their return values #27966
Conversation
packages/i18n/package.json
Outdated
@@ -27,6 +27,7 @@ | |||
}, | |||
"dependencies": { | |||
"@babel/runtime": "^7.11.2", | |||
"@wordpress/hooks": "^2.11.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@wordpress/hooks": "^2.11.0", | |
"@wordpress/hooks": "file:../hooks", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks!
Thanks for picking this up again. Note that there are still very legitimate performance concerns with this approach: #12517 (comment) A more performant way to let devs override strings might be to use |
@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:
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. |
packages/i18n/src/create-i18n.js
Outdated
/** | ||
* WordPress dependencies | ||
*/ | ||
import { applyFilters } from '@wordpress/hooks'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
orcreateHooks
function that creates aI18n
orHooks
object that has methods like__
orapplyFilters
- 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
__
andapplyFilters
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 } );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
packages/i18n/src/create-i18n.js
Outdated
* @param {string} domain Text domain. Unique identifier for retrieving translated strings. | ||
*/ | ||
return String( | ||
applyFilters( 'i18n.gettext', translation, text, domain ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
FYI for @yuliyan: this is related to your recent work on |
@yuliyan Are you actively working on |
@swissspidy In your client-side locale switching demo, did you use the The |
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! :) |
This is the outcome of the existing post-editor performance test against the current PR: This obviously will be triggering the
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. |
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 |
@jsnajdr Is anything else needed here, or can we go ahead and merge this? |
packages/i18n/src/create-i18n.js
Outdated
if ( typeof hooks === 'undefined' ) { | ||
return translation; | ||
} | ||
translation = String( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type definitions / JSDoc say that it returns a string, so it makes sense to uphold that promise, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(
~~~~~~~~~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsnajdr It looks like applyFilters
returns unknown
rather than any
which means (I think) that we can't just type hint it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leewillis77 I found that this workaround works:
translation = /** @type {string} */ ( /** @type {*} */ applyFilters( ... ) );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsnajdr Thanks. PR updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@adamsilverstein There is a good suggestion by @yuliyan in #27966 (comment) to remove the If this issue is resolved, I think we can merge the PR and celebrate 🎉 |
Thanks @leewillis77 for working on this! I'm going to merge the PR now 👍 |
@@ -0,0 +1,291 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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: |
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 🎉 |
@gziolo I'll work on a docs article today, and raise a separate PR (?) to add it. |
That sounds like an excellent plan ❤️ |
PR #28553 raised with markdown docs for the new filters. |
This reverts commit 736a005.
…"" This reverts commit 6469f66.
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 )