Venia Localization w/ i18n #2314
Conversation
- Calling Magento API endpoint with extracted phrases now - Refactored buildpack graphql to support variables
- Updated parse-translations to retrieve locales from magento, query for translations then export json translation files
- Moving fetch of available store views to build time for a) speed and b) simplicity - Rewrote to handle proper store code setting in graphql and local definition as different variables (store code does not always equal locale) - Added new cache key for apollo cache for each store view to handle storing responses properly per store view and not getting content flickering - Overall code tidiness improvements
- Removing redundant imports - Restoring comment location to original
magento-engcom-team
added
partners-contribution
Partner: Absolunet Inc
Event: Global-Contribution-Day
dpatil-magento
added
the
CI_APPROVED
LogERROR ON TASK: prettierCheck ERROR ON TASK: unitTests ERROR ON TASK: validateQueries ERROR ON TASK: scaffoldingSucceeds |
Lint and prettier fixes Small code improvements
|
Will this implementation support multisite? |
|
@chris-brabender First, I'm terribly sorry. Two months is a ridiculously long wait for such a generous contributions. Mea culpa; this really fell to me to start the review, and I spent the last two months too focused on extensibility drafts to give you good feedback. I'll start a review now--this comment is just to let you know that I've started. |
There was a problem hiding this comment.
Thank you so much. It was fun to go over this, and it took a while to set it all up and demo to myself, but that was my own fault.
This set of comments is just a start; I'm ready to have a meeting with you and @tjwiebell and @jimbo to go over the next steps for this. But here's what I think they would be.
- Address the big questions here about suspense and inlining of query responses
- Rebase this PR on develop to get access to targets
- Brainstorm what targets you could use today to integrate this
- For the missing pieces or the hard parts, request or implement the targets that would make things easier (e.g. for adding cache invalidation rules to
useQuery) - Eventually, refactor this as an extension and go to Disneyland (when it's open again I guess)
Once again, thanks. I'll follow up with y'all through other channels.
| ], | ||
| "dependencies": { | ||
| "i18next": "~19.1.0", | ||
| "react-i18next": "~11.3.3" | ||
| } |
There was a problem hiding this comment.
I think this belongs in devDependencies. The root repository isn't a production repo--the closest it comes to prod is that it's often used in deploys.
|
|
||
| /** Get and Set current store view */ | ||
| if (typeof persistence.getItem('store_view') === 'undefined' || typeof persistence.getItem('locale') === 'undefined') { | ||
| persistence.setItem('store_view', DEFAULT_STORE_VIEW.code); | ||
| persistence.setItem('locale', DEFAULT_STORE_VIEW.locale); | ||
| } | ||
|
|
||
| const routeCacheKey = 'urlResolver_' + persistence.getItem('store_view'); |
There was a problem hiding this comment.
That you have to do this tells me that we need this type of store state in a top-level ContextProvider. Did you try, at one point, to do this using React state patterns and found no place to put it? That's a missing feature in Peregrine, if so. Seems like the sensible place for it to go is a slice in the Redux store, for now. @jimbo might say that the app slice is for UI state and not content state, though, so if the team thinks app isn't appropriate, then we'd put it in something else. Maybe a ContextProvider and a reducer hook, instead of Redux.
There was a problem hiding this comment.
So I think the recommendation from you / the team was to steer away from ContextProviders. I did review it initially but did not in the end go with it because the context providers are added after the first call we need to the localisation service.
| @@ -89,17 +96,33 @@ function remotelyResolveRoute(opts) { | |||
| * @returns {Promise<{type: "PRODUCT" | "CATEGORY" | "CMS_PAGE"}>} | |||
| */ | |||
| function fetchRoute(opts) { | |||
| // String lang path from url resolver | |||
| const availableStoreViews = AVAILABLE_STORE_VIEWS; | |||
There was a problem hiding this comment.
The webpack.DefinePlugin will inline the object literal here and everywhere else this string goes. It's probably not a large object, but it'll bloat the bundle unless it's centralized in a constants module somewhere--plus, in a place like this, the whole object gets reallocated every time fetchRoute() is called.
| route = route.substring(path.length, route.length); | ||
| } | ||
| }); | ||
| route = route === '' ? '/' : route; |
There was a problem hiding this comment.
This wasn't working for me when I set up separate store views for French and Spanish. I tried it both with the "store views in URL path" options set and without. This may partly be a bug in urlResolver, but at any rate I'd like to know if/how you plan to support both use cases, with store view URLs turned on and without.
There was a problem hiding this comment.
For now the match is on the en_us pattern, so if you had just /fr or /es or /fr_FR it may not have worked properly.
There was a problem hiding this comment.
Ideally we could retrieve the paths from system config but that data isn't available via GraphQL for now.
| const _t = useCallback(phrase => { | ||
| return i18n.t(phrase); | ||
| }, []); |
There was a problem hiding this comment.
[deep breath]
This branch adds react-i18next as a dependency, but I don't see it in use anywhere. Were you using it at some point? If I remember right, you knew about react-i18next's useTranslation hook but chose to use the i18n client directly.
In your implementation, components retrieve translated text from this useLocalization hook. You could call useTranslation in this hook to get the _t function out, but instead you're just pulling the function from the i18n singleton. This is simple, but it prevents you from using the asynchronous-load feature of the useTranslation hook.
I know that this PR inlines all of the translations and therefore doesn't need to suspend to get them, but we know that that won't scale to large translation dictionaries. If we know we have to change the signature of useLocalization from sync to async eventually, let's do it now. Let's switch to useTranslation and do the refactoring work to make all the localized components deal with it a possible suspense properly. (We'll probably have to do this soon anyway.)
| const getTranslations = (local, phrases) => { | ||
| const variables = { locale: local, phrases: phrases }; | ||
| const query = graphQLQueries.getTranslations; | ||
| return fetchQuery(JSON.stringify({ query, variables })).then( | ||
| data => data.translate.items | ||
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Get Available Locales | ||
| */ | ||
| const getAvailableLocales = () => { | ||
| const query = graphQLQueries.getAvailableLocales; | ||
| return fetchQuery(JSON.stringify({ query })).then( | ||
| data => data.availableLocales.items | ||
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Get Available Store Views | ||
| */ | ||
| const getAvailableStoreViews = () => { | ||
| console.log('Fetching available store views'); | ||
| const query = graphQLQueries.getAvailableStoreViews; | ||
| return fetchQuery(JSON.stringify({ query })).then( | ||
| data => data.availableStoreViews.items | ||
| ); |
There was a problem hiding this comment.
This shows a clear intention to hardcode the available translations and store views into the bundle. It's fast and simple, but it does mean that all that data gets locked in at build time and can't change without a rebuild.
I know we discussed this extensively in other channels. Now that I see it written out, I think there's a clear use case for a Webpack plugin here which preloads query results at build time in a way that the client app can still fetch the results with useQuery. With parameterized queries, that's a little harder, but most of these queries have no dependencies on an app context. It would be interesting to think about a general case for GraphQL prefetch at build time.
(I know this kinda turns into a circular dependency at build time, or a zig-zag dependency graph where production depends on staging, etc. It's still obviously the right thing to do in some cases, and I'm glad you did it.)
There was a problem hiding this comment.
I don't see translations changing without deployments when working with traditional magento sites to be honest. I don't think we neeed to be able to fetch this dynamically. But we can explore options. The issue is loading up a page where the language is supposed to be set (say when entering a route specifically) and having an english block shown while it is still fetching a french/spanish translation.
| module.exports.command = 'parse-translations <directory>'; | ||
|
|
||
| module.exports.describe = | ||
| 'Load and validate the current environment, including .env file if present, to ensure all required configuration is in place.'; | ||
|
|
||
| module.exports.builder = { | ||
| coreDevMode: { | ||
| type: 'boolean', | ||
| desc: | ||
| 'For core @magento/pwa-studio repository development. Creates a .env file populated with examples if one is not present.' | ||
| } | ||
| }; |
There was a problem hiding this comment.
I know you left these in when copying another CLI as a template, but it still threw me for a loop.
| results.forEach(function(result) { | ||
| parser.parseFuncFromString(fs.readFileSync(result), { list: ['_t'] }); | ||
| }); |
There was a problem hiding this comment.
Brilliant! This is a beautiful feature of i18next used very well. While I appreciate you making a new Buildpack command (really, this is the first one I've seen, and it's great), I wonder if this could be done as a target integration with the Webpack compiler.
Webpack parser hooks let you take advantage of the parse that Webpack already does. The https://github.com/Perlmint/ya-i18next-webpack-plugin uses that feature; I forget if we discussed this plugin already. It would be great to automatically gather (and aggressively cache) these translations as part of a normal build, rather than as a separate command to remember to run.
There was a problem hiding this comment.
I don't think that plugin has been brought up before as I was more looking at react i18n rather than webpack. But looks good initially.
| query { | ||
| availableStoreViews { | ||
| items { | ||
| code | ||
| name | ||
| locale | ||
| is_default | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Your PR to Magento 2 didn't have this in the schema, but I was able to mock it out fine.
There was a problem hiding this comment.
There was a second PR for this I thought, one for StoreConfig and one for Translations. The second one should have had this in it.
There was a problem hiding this comment.
Looks like that PR never got submitted. My bad.
| * Merge Venia UI and Local Resources together | ||
| * We can add additional resources here (maybe from peregrine for example) | ||
| * Would want to look at exposing this to the extensibility work also | ||
| */ | ||
| const resources = merge(resourcesVeniaUI, resourcesLocal); |
There was a problem hiding this comment.
Yes! We absolutely want to merge and override here. Seems like the pattern here is already most of the way there; you've got a loader which renders this data as JSON, and you could change it to codegen which yields the JSON directly. To match a pattern which we've used elsewhere:
- Replace the two calls to Venia and local with a single call:
import resources from '@absolunet/pwa-i18n/magic-resources'or something. - Implement
magic-resourcesto assemble the localization files in all dependencies - Use the new
transformModulesTarget to apply aval-loaderto that file only
You could call your own targets in there, exposing your own localization extension points.
4.
|
Should be closed because is already totally different implemented and can not merge and makes no sense to merge this |
|
@magento assign extra credits to @chris-brabender for leading the design and research on i18n implementation: 80 points |
Description
Introduced localised content to PWA Studio.
Note: draft PR for feedback and comments
Related Issue
#669
Acceptance
Verification Stakeholders
Specification
Verification Steps
Screenshots / Screen Captures (if appropriate)
Checklist