-
Notifications
You must be signed in to change notification settings - Fork 2k
I18N: Fix the lib/wp was called before locale data was ready #107124
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
@jsnajdr I would appreciate it if you could review this and share any concerns or feedback regarding this approach 🙏 |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
jsnajdr
left a comment
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.
Yes, this makes sense to me 🙂
alternative approach would be to fallback to
_locale: 'user'; however, it is unclear whether this might cause issues for logged-out users.
I agree, _locale: 'user' wouldn't work for logged out users, e.g., on the login and signup pages, and other "landing" pages like accept invite etc.
client/state/ui/language/actions.ts
Outdated
| type: LOCALE_SET, | ||
| localeSlug, | ||
| localeVariant, | ||
| await switchLocale( newLocale ); |
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.
Why did you change the order of switchLocale and dispatch in this function?
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.
Previously, we couldn’t guarantee the order because switchLocale sends requests to fetch the locale data, but we weren’t waiting for the response. As a result, I’m not entirely sure this "changes" the order.
Do you think we should now ensure that the locale data is fully loaded before dispatching the action to update the locale in the Redux store?
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.
Yes, it makes more sense to me if the entire app state (i18n libraries, Redux, DOM state set in setLocaleInDOM) is switched synchronously at once.
With the flipped order, the Redux state would be set first and then the rest only after a while. That doesn't sound very reliable.
| } | ||
| } else { | ||
| getLanguageFile( localeSlug ).then( | ||
| await getLanguageFile( localeSlug ).then( |
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.
As a code quality improvement, it would be nice to rewrite this await+then mix into pure try/catch/await code.
Part of I18N-217
Proposed Changes
_localeparameter from being injected correctly.lib/wpreceives the correct locale._locale: 'user'; however, it is unclear whether this might cause issues for logged-out users.Why are these changes being made?
Testing Instructions
/sites/:sitePlanis rendered as EnglishPlanis renders as your current languagePre-merge Checklist