Skip to content
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

Venia Localization w/ i18n #2314

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,9 @@
"path": "./packages/venia-concept/dist/+([0-9]).*.js",
"maxSize": "100 kB"
}
]
],
"dependencies": {
"i18next": "~19.1.0",
"react-i18next": "~11.3.3"
}
Comment on lines +104 to +108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
29 changes: 26 additions & 3 deletions packages/peregrine/lib/Router/resolveUnknownRoute.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ import BrowserPersistence from '../util/simplePersistence';
* @param {{ route: string, apiBase: string, __tmp_webpack_public_path__: string}} opts
*/
const persistence = new BrowserPersistence();
const routeCacheKey = 'urlResolver';

/** 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');
Comment on lines +8 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Some M2.3.0 GraphQL node IDs are numbers and some are strings, so explicitly
// cast numbers if they appear to be numbers
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const langs = availableStoreViews.map(item => {
return item.locale;
});
let route = opts.route;

langs.forEach(function(lang) {
const path = '/' + lang.toLowerCase();
if (route.startsWith(path) || route == path) {
route = route.substring(path.length, route.length);
}
});
route = route === '' ? '/' : route;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we could retrieve the paths from system config but that data isn't available via GraphQL for now.


const url = new URL('/graphql', opts.apiBase);
return fetch(url, {
method: 'POST',
credentials: 'include',
headers: new Headers({
'Content-Type': 'application/json'
'Content-Type': 'application/json',
STORE: persistence.getItem('store_view')
}),
body: JSON.stringify({
query: `
{
urlResolver(url: "${opts.route}") {
urlResolver(url: "${route}") {
type
id
}
Expand Down
95 changes: 95 additions & 0 deletions packages/peregrine/lib/hooks/useLocalization.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { useMemo, useState, useCallback } from 'react';
import { Util, useToasts } from '@magento/peregrine';
import i18n from 'i18next';
const { BrowserPersistence } = Util;
const storage = new BrowserPersistence();

export const useLocalization = () => {
const [, { addToast }] = useToasts();

const availableStoreViews = AVAILABLE_STORE_VIEWS;
const availableLangs = availableStoreViews.map(item => {
return item.locale.toLowerCase();
});
Comment on lines +10 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks invariant, it probably can go outside the hook function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had just planned on useLocalisation as a global catch all but thats a bad idea. We can create a new service class for this or something.


const [currentLocale, setCurrentLocale] = useState(
storage.getItem('locale')
);
const [currentStoreView, setCurrentStoreView] = useState(
storage.getItem('store_view')
);
Comment on lines +15 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Together, this seems like a case for useReducer, since those two values are very dependent on one another.

const localizationState = {
currentLocale,
currentStoreView,
availableLangs,
availableStoreViews
};

const _t = useCallback(phrase => {
return i18n.t(phrase);
}, []);
Comment on lines +28 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.)


/**
* Handling switch store via redirect / refresh for now
* @TODO Find a more elegant solution to refresh all graphql queries with new header store code
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can propagate naturally through the components that use Peregrine talons. Right now, most Peregrine hooks use useQuery and useLazyQuery directly out of @apollo/react-hooks. We can put a facade in front of those functions that lists the store view state as a useEffect dependency. Then, every component which ultimately calls useQuery would trigger a re-render when the store view changed.

That still might be a pretty expensive cascading render, when many query results wouldn't change at all. What might be better would be a wrapper (or even an Apollo Link) which uses the "broadcasting" feature of Apollo Client.

The wrtiteQuery function is supposed to intelligently force updates to any queries which have used the changed values. Venia doesn't rely on this because in Apollo 2.6 it doesn't work that well.

But there is a lot of work in Apollo Client 3.0 beta on this!
apollographql/apollo-client#6046
apollographql/apollo-client#6221

As of this comment, those PRs were only merged a month ago, so we can look forward to this stuff being easier with an Apollo Client update.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't see an easy to have this update all queries easily. If there is something available it wold be great though. Basically every query needs to be re-fetched with the new STORE header.
But we need to work on how we can account for changes in URL keys for the new store view too. EG: Product XYZ has url-key /product-english.html in STORE=en_us but /produit-francais.html in STORE=fr_fr so a redirect with canonicalisation should be happening here potentially.

*/
const changeLanguage = useCallback((item) => {
storage.setItem('store_view', item.code);
storage.setItem('locale', item.locale);

setCurrentLocale(item.locale);
setCurrentStoreView(item.code);
i18n.changeLanguage(item.locale.toLowerCase());

addToast({
type: 'info',
message: _t(
`Switching Store View to ${item.name}, the page will reload briefly`
),
timeout: 3000
});

window.location.replace(
`/${storage.getItem('locale').toLowerCase()}`
);
}, [addToast, _t])

Comment on lines +44 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the place where we'd want to only do this if the URL needs to change based on locale. (Unless we don't want to support the other case at all)

/**
* Handles switching store by store code (in switcher component)
*/
const handleSwitchStore = useCallback(
code => {
const newStoreView = availableStoreViews.reduce(function(acc, item) {
return acc !== false ? acc: (item.code == code ? item : false);
}, false);
Comment on lines +62 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is equivalent to availableStoreViews.find(item => item.code === code). Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like it could work!


changeLanguage(newStoreView);
},
[availableStoreViews, changeLanguage]
);

/**
* Handles switching store by locale (eg: in route)
*/
const handleSwitchStoreByLocale = useCallback(
locale => {
const newStoreView = availableStoreViews.reduce(function(acc, item) {
return acc !== false ? acc: (item.locale == locale ? item : false);
}, false);

changeLanguage(newStoreView);
},
[availableStoreViews, changeLanguage]
);

const api = useMemo(
() => ({
handleSwitchStore,
handleSwitchStoreByLocale,
_t
}),
[handleSwitchStore, handleSwitchStoreByLocale, _t]
);

return [localizationState, api];
};
1 change: 1 addition & 0 deletions packages/peregrine/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export { useRestApi } from './hooks/useRestApi';
export { useRestResponse } from './hooks/useRestResponse';
export { useScrollLock } from './hooks/useScrollLock';
export { useSearchParam } from './hooks/useSearchParam';
export { useLocalization } from './hooks/useLocalization';
export {
WindowSizeContextProvider,
useWindowSize
Expand Down
1 change: 1 addition & 0 deletions packages/peregrine/lib/store/reducers/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const initialState = {
isOnline: navigator.onLine,
overlay: false,
searchOpen: false,
query: '',
pending: {}
};

Expand Down
5 changes: 5 additions & 0 deletions packages/peregrine/lib/talons/Navigation/useNavigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const ancestors = {
FORGOT_PASSWORD: 'SIGN_IN',
MY_ACCOUNT: 'MENU',
SIGN_IN: 'MENU',
SWITCH_STORE: 'MENU',
MENU: null
};

Expand Down Expand Up @@ -72,6 +73,9 @@ export const useNavigation = props => {
const showSignIn = useCallback(() => {
setView('SIGN_IN');
}, [setView]);
const showSwitchStore = useCallback(() => {
setView('SWITCH_STORE');
}, [setView]);

return {
catalogActions,
Expand All @@ -88,6 +92,7 @@ export const useNavigation = props => {
showMainMenu,
showMyAccount,
showSignIn,
showSwitchStore,
view
};
};
20 changes: 20 additions & 0 deletions packages/peregrine/lib/talons/SwitchStore/useSwitchStore.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { useCallback } from 'react';

/**
* Returns props necessary to render an SwitchStore component.
*
* @param {object} props
* @param {function} props.showSwitchStore - callback that displays sign in view
* @return {{
* handleSwitchStore: function
* }}
*/
export const useSwitchStore = props => {
const { showSwitchStore } = props;

const handleSwitchStore = useCallback(() => {
showSwitchStore();
}, [showSwitchStore]);

return { handleSwitchStore };
};
Comment on lines +1 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for faithfully replicating the talon pattern. I notice here that this talon is a really common, simple transformation of one named property into another. When you wrote the SwitchStore component, did it help to explicitly write this out in the talon file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really was just following the pattern. Gives people the ability to customise it easily in the future also I guess?

45 changes: 41 additions & 4 deletions packages/pwa-buildpack/lib/Utilities/graphQL.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const fetchQuery = query => {
'Content-Type': 'application/json',
'Accept-Encoding': 'gzip'
},
body: JSON.stringify({ query }),
body: query,
agent: fetchAgent
}
)
Expand All @@ -33,7 +33,8 @@ const fetchQuery = query => {
* @returns Promise that will resolve to the media backend url.
*/
const getMediaURL = () => {
return fetchQuery(graphQLQueries.getMediaUrl).then(
const query = graphQLQueries.getMediaUrl;
return fetchQuery(JSON.stringify({ query })).then(
data => data.storeConfig.secure_base_media_url
);
};
Expand All @@ -42,7 +43,40 @@ const getMediaURL = () => {
* Get the schema's types.
*/
const getSchemaTypes = () => {
return fetchQuery(graphQLQueries.getSchemaTypes);
const query = graphQLQueries.getSchemaTypes;
return fetchQuery(JSON.stringify({ query }));
};

/**
* Get translations
*/
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
);
Comment on lines +53 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

/**
Expand All @@ -64,5 +98,8 @@ const getUnionAndInterfaceTypes = () => {
module.exports = {
getMediaURL,
getSchemaTypes,
getUnionAndInterfaceTypes
getTranslations,
getAvailableLocales,
getUnionAndInterfaceTypes,
getAvailableStoreViews
};
Loading