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

react-i18n perf optimizations #659

Merged
merged 6 commits into from
Apr 17, 2019
Merged

react-i18n perf optimizations #659

merged 6 commits into from
Apr 17, 2019

Conversation

lemonmade
Copy link
Member

This PR addresses a few performance concerns, one of which was the cause of the issue we had in Web when we tried to roll these out:

  1. Calling subscriptions when async translations are resolved now queues subscriptions to run in a single animation frame
  2. When async translations are resolved to undefined, we no longer call the subscription (this is technically a bit dangerous because the loading property can get out of date, but we don't currently use this anywhere anyways, and this basically only happens for more detailed locales (e.g., fr-CA), where the consumer will get an update on the fr part resolving just fine
  3. I removed a hacky useSimpleI18n hook and, instead, made useI18n bail out early when there are no options passed. This violates the rules of hooks, but we get around that by throwing an error if they try to switch between the two modes
  4. I split apart the context for i18n objects and the one for the nested IDs in the parent. By having useI18n hooks linked up to the I18nContext, any update to one i18n object higher in the tree would cause all descendants' hooks to run again, which would be useless, because the IDs in question are still the same.
  5. Made it so that the ShareTranslations component that gets returned is always the same function. This was the main cause of the incident we had: when the "highest" i18n-connected component had some translations that resolved async (even for locales like en-CA, which would do an async lookup for the -CA languages), its ShareTranslations would change. When that happened, the entire subtree below it got unmounted (this is how React's reconciliation works), then remounted. For a component like our AdminNextFallback, which sometimes makes non-idempotent get requests on mount, this could cause issues (like multiple GETs eventually causing full-page auth redirects).

With these in place, the performance of i18n on load is much better. Here's the before/ after for a shop in en-CA:

Screen Shot 2019-04-17 at 10 46 14 AM

Screen Shot 2019-04-17 at 10 52 15 AM

The hydrate bit is about the same length in both, So this basically shaves off 500–600ms.

Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

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

A few small comments but this looks rad, 500ms is a big deal :)

packages/react-hooks/README.md Show resolved Hide resolved
packages/react-i18n/src/hooks.tsx Show resolved Hide resolved
packages/react-i18n/src/hooks.tsx Show resolved Hide resolved
const isBrowser = typeof window !== 'undefined';
const enqueue = isBrowser ? window.requestAnimationFrame : setImmediate;

this.enqueuedUpdate = enqueue(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

neat

yarn.lock Outdated Show resolved Hide resolved
@lemonmade lemonmade merged commit 61e8236 into master Apr 17, 2019
@lemonmade lemonmade deleted the i18n-perf-optimizations branch April 17, 2019 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants