-
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
Add react-i18n package with i18n React bindings #28465
Conversation
packages/react-i18n/src/index.jsx
Outdated
// rerender translations whenever a hook is removed or added | ||
useEffect( () => { | ||
hooks.addAction( 'hookAdded', 'core/react-i18n/filters', () => | ||
forceUpdate() | ||
); | ||
hooks.addAction( 'hookRemoved', 'core/react-i18n/filters', () => | ||
forceUpdate() | ||
); | ||
return () => { | ||
hooks.removeAction( 'hookAdded', 'core/react-i18n/filters' ); | ||
hooks.removeAction( 'hookRemoved', 'core/react-i18n/filters' ); | ||
}; | ||
}, [] ); |
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.
Should filtering perhaps better be handled by #27966?
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 is working on adding filters to the i18n package. I would be curious to learn how much those two PRs overlap.
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.
That's a question for @yuliyan, who added this feature in Automattic/wp-calypso#44743, presumably to implement a i18n feature like Community Translator. @yuliyan did you implement the feature you needed the hooks for yet?
And would it be ok if these hooks were not in the react-i18n
library, but directly in the I18n
instance in @wordpress/i18n
?
Another difference is that @leewillis77's work in #27966 and also the Core translate
PHP function only have hooks to modify the gettext
result after the translation, and not its arguments before translation. @yuliyan's version has a preTranslation
hook that can modify these arguments.
I'd personally prefer to move the hooks support to the core I18n
class and simplify the React bindings as much as possible. But we need to make sure that the features that depend on these hooks are OK with that.
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.
That's a question for @yuliyan, who added this feature in Automattic/wp-calypso#44743, presumably to implement a i18n feature like Community Translator.
That's correct, but the implementations are still in a draft state and have not been released yet.
And would it be ok if these hooks were not in the
react-i18n
library, but directly in theI18n
instance in@wordpress/i18n
?
I think it would be even better to have them directly in the I18n
instance, though react-i18n
bindings should still maintain reactivity when adding/removing hooks.
Another difference is that @leewillis77's work in #27966 and also the Core
translate
PHP function only have hooks to modify thegettext
result after the translation, and not its arguments before translation. @yuliyan's version has apreTranslation
hook that can modify these arguments.
I like the implementation in #27966 as it looks closer to the core PHP gettext
function filters and essentially provides the same functionality as the postTranslation
filter here.
As for the lack of preTranslation
- I can't tell if it's a mandatory feature, but it would definitely come in handy. The first use case that pop into my head would be to use it for implementing a different translation lookup logic, in case the JED file uses translation keys other than the original string (e.g. hashes, ids, etc.).
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 think it would be even better to have them directly in the
I18n
instance
Perfect! That means I can remove them from the react-i18n
bindings in this PR.
though
react-i18n
bindings should still maintain reactivity when adding/removing hooks.
That can be implemented in i18n
by registering the hookAdded
/hookRemoved
callbacks and fire a subscribe
event when a relevant i18n.*
hook changes. At this moment, subscribe
listeners are notified only on setLocaleData()
calls, now they'll also be notified when hooks change.
As for the lack of
preTranslation
- I can't tell if it's a mandatory feature, but it would definitely come in handy. The first use case that pop into my head would be to use it for implementing a different translation lookup logic, in case the JED file uses translation keys other than the original string
OK, but that sounds like a rather hypothetical use case to me, not something we need in near future. Or is it needed for any of the existing i18n-calypso
hooks and filters that we use, e.g., for Community Translator or for highlighting untranslated strings in the UI?
The pre_gettext
hooks can be added at any later time to @wordpress/i18n
.
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.
That can be implemented in
i18n
by registering thehookAdded
/hookRemoved
callbacks and fire asubscribe
event when a relevanti18n.*
hook changes. At this moment,subscribe
listeners are notified only onsetLocaleData()
calls, now they'll also be notified when hooks change.
Sounds good!
Or is it needed for any of the existing
i18n-calypso
hooks and filters that we use, e.g., for Community Translator or for highlighting untranslated strings in the UI?
No, it's not needed for any of the existing features. Although, i18n-calypso
supports the mentioned translation lookup feature, we are not currently using it.
If you'd still like to go this route, there are a couple of options you may not have considered:
|
@sirreal I'd love to add the TypeScript support back, but it was a too big task for me to do everything at once. I'll try to re-add the TS syntax after the dust settles on other parts of this PR 🙂 |
packages/i18n/src/create-i18n.js
Outdated
@@ -116,6 +145,19 @@ export const createI18n = ( initialData, initialDomain ) => { | |||
...DEFAULT_LOCALE_DATA[ '' ], | |||
...tannin.data[ domain ][ '' ], | |||
}; | |||
|
|||
listeners.forEach( ( listener ) => listener() ); |
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 make sense to use doAction
here instead?
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.
Something like doAction( 'i18n.update' )
? That's an interesting idea! 👍
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.
Is this still a to-do?
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 tried to implement @yuliyan's suggestion with the doAction
hook, but then found it's not practical to work with.
With the hook, it's no longer enough to pass an I18n
instance as a prop the I18nProvider
. I also need to pass along an instance of the Hooks
object, the one that was used to construct that I18n
instance. Because there's no direct way to subscribe to the I18n
instance, I need to go indirectly through Hooks
.
Also, what if I have multiple instances of I18n
created with the same instance of Hooks
? That's going to happen in the "real-time locale switcher" use case. Then the doAction
hook would need to receive the I18n
instance as a parameter, and the callback would need to check if the action fired for our instance, or for someone else.
In the end, I'm finding a I18n.subscribe
method to be more practical.
dab38d7
to
d5baff2
Compare
Size Change: -26 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
@leewillis77 I'll try to add the filter. I'll need to figure out which filter to call: |
I think it should be a new filter, something like:
Type annotations are probably required on the return value from applyFilters. The only other complication is that unsupplied values are passed as undefined to the other filters, whereas it looks like you'd pass "default" here (e.g. for WordPress core strings which have no text-domain). |
4d1f992
to
d45668a
Compare
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.
Very exciting to see this coming together, I left a few quick notes.
I was working on some tooling updates to support TypeScript but haven't been able to finish. It would be nice to get those landed: #27143
packages/react-i18n/tsconfig.json
Outdated
"compilerOptions": { | ||
"rootDir": "src", | ||
"declarationDir": "build-types", | ||
"jsx": "preserve" |
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.
Because we don't rely on TypeScript compiling jsx, I think we can move this to the base config.
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.
Turns out the jsx
options already is in the base config. I don't know why I felt I need to add it here, too. Removed in the latest commit.
I feel like the TS build tooling changes should be done in a separate PR. Does the rest of the core team agree that this move to TS makes sense? Aside: why not |
I agree, that would be nice to isolate.
TypeScript requires TypeScript files that include JSX to use the
This is very minor in practice, but does require using |
I agree, I'm trying to make the entire package work end-to-end and figure out what it takes. Then we can merge independent parts incrementally.
I was getting TypeScript compiler errors where it couldn't recognize the JSX syntax unless the file had a |
dec8018
to
3013341
Compare
OK, as @sirreal explains, the |
The Javascript standards don't seem to be clear on case selection for filter names. The closest I can find would be this which suggests "camel case with a lower case first letter": If that seems sensible then I'll:
|
@leewillis77 The coding standard specifies only names for variables and functions, not for hooks identifiers, as far as I see. Consistency with WordPress Core PHP hook names is a very strong argument in favor of |
@leewillis77 FYI that I renamed the |
Status of this PR: All the changes I wanted here are done and all checks are green. Time to start merging this step-by-step. There are three broad areas where this PR modifies things:
|
packages/i18n/src/create-i18n.js
Outdated
@@ -395,7 +395,7 @@ export const createI18n = ( initialData, initialDomain, hooks ) => { | |||
*/ | |||
result = /** @type { boolean } */ ( | |||
/** @type {*} */ hooks.applyFilters( | |||
'i18n.hasTranslation', | |||
'i18n.has_translation', | |||
result, | |||
single, | |||
context, |
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.
"domain" here is still being passed to filters as "default" rather than "undefined" for calls with no domain due to the default value in the method signature. That's different behaviour from the other i18n.
filters so would be nice to standardise.
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.
Thanks for the reminder, fixed in 43c0fff 🚀
bc30953
to
43c0fff
Compare
43c0fff
to
fefb81b
Compare
e494dab
to
76cb431
Compare
6344596
to
2a1e369
Compare
34bd668
to
d27d538
Compare
@swissspidy This dream has finally come true 🙂 The |
Yay :-) nice work! Looks good to me even though I personally wouldn't have written it in TS in the first place :-) |
Thanks everyone for collaboration ❤️ Going to merge the PR now. |
Adds
react-i18n
package inspired by @sirreal's work in Calypso and by experimental locale switching by @swissspidy in #27973.@sirreal and @swissspidy are the actual authors of the code I would say, I act mostly as a copy&paster here 🙂
The
react-i18n
bindings read from theI18n
instance, supplied as a prop to the context provider, and makes the i18n functions available to children React components that use the consumer hook.I added some methods to
@wordpress/i18n
that are needed by thereact-i18n
bindings for a concise and hack-free implementation:defaultI18n
instance, so that we can pass it around as a whole.getLocaleData
method.hasTranslation
method.subscribe
method that notifies listeners about changes made byi18n.setLocaleData
.The
@wordpress/hooks
package gets adefaultHooks
export, again, so that we can pass it around as a parameter.The React bindings automatically rerender dependent components when the
i18n
instance changes, when new data are set on it withi18n.setLocaleData
, or when a pre/post-translation hook is added or removed (cc @yuliyan for review).Types:
I originally wanted to add native TypeScript sources for
react-i18n
, but ran into unsolvable problems when I tried. One of them are insufficient typings for imported packages. The Gutenberg monorepo doesn't have types forcreateHigherOrderComponent
from@wordpress/compose
, so I was unable to use it in a type-safe way.