-
Notifications
You must be signed in to change notification settings - Fork 2k
Hosting Dashboard: Introduce I18nProvider to make translation work correctly #103299
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
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 |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~498 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. 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. Generated by performance advisor bot at iscalypsofastyet.com. |
I feel this may be the better direction. The I did a brief exploration recently to just take out the relevant bits that bring the translation chunks (not in a "modernized" way). I wanted to explore instantiating the translate hook for another locale. You may or may not find this informative: https://github.com/Automattic/wp-calypso/pull/101444/files#diff-d4183e821955b796c8041d31d47959116b9077af3ee4707a110703ef9ff8eed0 (and FWIW - it worked) But overall, I think if we can isolate the "fetch and cache translations for a given locale" in a way that these can be loaded into / used for any ad hoc i18n provider, then I feel that would be gold. |
One thing I want to note too is that all these tools were built in a time where switching local without reloading the page was deemed mandatory to support. I wonder if we could make things a lot simpler by avoiding this requirement. |
Proposed #103333 to isolate the logic for "fetching and caching translations for a given locale" by abstracting the i18n implementation. With this change, we no longer need to rely on both Any thoughts? |
I think you shouldn't reuse the existing code in You'll be better off if you do it from scratch for the hosting dashboard. You should understand from first principles what's really needed to set up a locale:
Setting up localization should be in principle very simple, so let's avoid all the unnecessary complexity. As an illustration of the complexity, I still have three very old PRs open that tried to clean it up a little bit, but they all failed and I never finished them: #58172, #58343, #58345.
The main part of switching locale at runtime is using reactive |
Maybe @yuliyan might have some thoughts to add on this one. |
@arthur791004 @youknowriad I'm hesitant for Lego to rewrite the i18n given that we don't really have expertise in this area, and would rather see progress being done on the sites settings as discussed with the team. Would it make more sense to ask global team to pick this up? And what's the overhead of directly using i18n-calypso? |
I think @jsnajdr's comment covers everything pretty well. I'd personally go with his suggestion about using our own implementation to set up the locale, rather than reusing the setup from the main app. Happy to help if you need any directions or testing when you're ready with the implementation. |
I'll try to write some basic i18n bootstrap code, of the kind I recommended in an earlier comment. Let's see if it was really a good idea 🙂 |
This aligns with my understanding as well. I initially considered starting from scratch, but I realized that's essentially what setupLocale is doing—it just consolidates the intermediate results together with i18n-calypso. Additionally, I wasn't sure whether we still need to support some older features like empathy mode, which is why I chose to abstract the i18n layer to avoid making big changes 😅 |
import { useAuth } from '../auth'; | ||
|
||
export function I18nProvider( { children }: { children: React.ReactNode } ) { | ||
const { user } = useAuth(); |
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 checked out this locally. I found that the new language I set in /me
isn't reflected here unless I comment this line out 🥲 are you experiencing the same?
staleTime: 30 * 60 * 1000, // Consider auth valid for 30 minutes |
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.
We probably need to invalidate React Query caches for anything that depends on /me
information when we know that we've changed that information (i.e. updated the language in the interface).
https://tanstack.com/query/latest/docs/framework/react/guides/query-invalidation
@@ -26,7 +26,7 @@ export function getLocaleFromQueryParam() { | |||
return query.get( 'locale' ); | |||
} | |||
|
|||
export const setupLocale = ( currentUser, reduxStore ) => { | |||
export const setupLocale = ( currentUser, reduxStore = null ) => { |
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 reduxStore
optionality feels a bit suboptimal here 😬 It's not clear what it means: is it because the global Redux store has not been initialized yet, or because we don't intend to have one at all? Definitely feels like a code smell to avoid.
There could be some opportunities for decoupling and reusability, but in general, I agree with what @jsnajdr suggested above, which is, building a simpler version that works with the constraints at hand.
Closing this, as we’ve decided to rewrite the locale functionalities into smaller, composable pieces to better support Dashboard v2. |
Part of DOTCOM-10589
Proposed Changes
I18nProvider
under/dashboard
to setup the localereduxStore
argument in thesetupLocale
function optional, and have the function return the resolved locale.Needs to be discussed:
CalypsoI18nProvider
?i18n-calypso
?Both setupLocale and switchLocale functions are tightly coupled with
i18n-calypso
. If we don't want to usei18n-calypso
, we need to decouple the logic that fetches the locale data (enabled or disabled translation chunks) and then initial the locale data by@wordpress/i18n
on v2 dashboard.Why are these changes being made?
Testing Instructions
Pre-merge Checklist