From 61e8236a28b1251c52c09b24cfdb093beeb52ce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Sauv=C3=A9?= Date: Wed, 17 Apr 2019 14:20:52 -0400 Subject: [PATCH] react-i18n perf optimizations (#659) --- packages/react-hooks/CHANGELOG.md | 8 +- packages/react-hooks/README.md | 16 +++ packages/react-hooks/src/hooks/index.ts | 1 + packages/react-hooks/src/hooks/lazy-ref.ts | 13 ++ .../src/hooks/tests/lazy-ref.test.tsx | 37 ++++++ packages/react-i18n/CHANGELOG.md | 6 + packages/react-i18n/README.md | 32 +---- packages/react-i18n/package.json | 1 + packages/react-i18n/src/context.ts | 3 +- packages/react-i18n/src/hooks.tsx | 112 ++++++++++++------ packages/react-i18n/src/i18n.ts | 1 - packages/react-i18n/src/index.ts | 2 +- packages/react-i18n/src/manager.ts | 41 +++++-- packages/react-i18n/src/test/manager.test.ts | 55 ++++++++- yarn.lock | 24 ++-- 15 files changed, 261 insertions(+), 91 deletions(-) create mode 100644 packages/react-hooks/src/hooks/lazy-ref.ts create mode 100644 packages/react-hooks/src/hooks/tests/lazy-ref.test.tsx diff --git a/packages/react-hooks/CHANGELOG.md b/packages/react-hooks/CHANGELOG.md index ca538abd28..f231d5a14c 100644 --- a/packages/react-hooks/CHANGELOG.md +++ b/packages/react-hooks/CHANGELOG.md @@ -7,7 +7,13 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] -## [1.0.1] - 2019-04-17 +## [1.1.0] - 2019-04-17 + +### Added + +- Added a `useLazyRef` hook ([#659](https://github.com/Shopify/quilt/pull/659)) + +## [1.0.0] - 2019-04-12 ### Added diff --git a/packages/react-hooks/README.md b/packages/react-hooks/README.md index 5541dc83da..ac76ccacec 100644 --- a/packages/react-hooks/README.md +++ b/packages/react-hooks/README.md @@ -40,3 +40,19 @@ function MyComponent() { return
{foo}
; } ``` + +### `useLazyRef()` + +This hook creates a ref object like React’s `useRef`, but instead of providing it the value directly, you provide a function that returns the value. The first time the hook is run, it will call the function and use the returned value as the initial `ref.current` value. Afterwards, the function is never invoked. You can use this for creating refs to values that are expensive to initialize. + +```tsx +function MyComponent() { + const ref = useLazyRef(() => someExpensiveOperation()); + + React.useEffect(() => { + console.log('Initialized expensive ref', ref.current); + }); + + return null; +} +``` diff --git a/packages/react-hooks/src/hooks/index.ts b/packages/react-hooks/src/hooks/index.ts index 5b9786b277..96a155cfd4 100644 --- a/packages/react-hooks/src/hooks/index.ts +++ b/packages/react-hooks/src/hooks/index.ts @@ -1,2 +1,3 @@ +export {useLazyRef} from './lazy-ref'; export {default as useTimeout} from './timeout'; export {default as useOnValueChange} from './on-value-change'; diff --git a/packages/react-hooks/src/hooks/lazy-ref.ts b/packages/react-hooks/src/hooks/lazy-ref.ts new file mode 100644 index 0000000000..ed63786738 --- /dev/null +++ b/packages/react-hooks/src/hooks/lazy-ref.ts @@ -0,0 +1,13 @@ +import {useRef, MutableRefObject} from 'react'; + +const UNSET = Symbol('unset'); + +export function useLazyRef(getValue: () => T): MutableRefObject { + const ref = useRef(UNSET); + + if (ref.current === UNSET) { + ref.current = getValue(); + } + + return ref as MutableRefObject; +} diff --git a/packages/react-hooks/src/hooks/tests/lazy-ref.test.tsx b/packages/react-hooks/src/hooks/tests/lazy-ref.test.tsx new file mode 100644 index 0000000000..a879804099 --- /dev/null +++ b/packages/react-hooks/src/hooks/tests/lazy-ref.test.tsx @@ -0,0 +1,37 @@ +import * as React from 'react'; +import {mount} from '@shopify/react-testing'; + +import {useLazyRef} from '../lazy-ref'; + +describe('useLazyRef()', () => { + it('calls the passed function and returns the ref for it', () => { + const content = 'Hello world'; + const spy = jest.fn(() => content); + + function MockComponent() { + const value = useLazyRef(spy); + return
{value.current}
; + } + + const mockComponent = mount(); + + expect(spy).toHaveBeenCalled(); + expect(mockComponent).toContainReactText(content); + }); + + it('does not call the passed function on subsequent renders', () => { + function MockComponent({spy}: {spy: () => any}) { + const value = useLazyRef(spy); + return
{value.current}
; + } + + const mockComponent = mount( 'Hello'} />); + + const newContent = 'Goodbye'; + const spy = jest.fn(() => newContent); + mockComponent.setProps({spy}); + + expect(spy).not.toHaveBeenCalled(); + expect(mockComponent).not.toContainReactText(newContent); + }); +}); diff --git a/packages/react-i18n/CHANGELOG.md b/packages/react-i18n/CHANGELOG.md index b1b00dd122..f641188730 100644 --- a/packages/react-i18n/CHANGELOG.md +++ b/packages/react-i18n/CHANGELOG.md @@ -8,6 +8,12 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ## [Unreleased] +### Fixed + +- Fixed a number of performance issues with resolving asynchronous translations ([#659](https://github.com/Shopify/quilt/pull/659)) + +## [1.1.1] - 2019-04-12 + ### Changed - `Weekdays` renamed to `Weekday` as part of `shopify/typescript/prefer-singular-enums` eslint rule ([#648](https://github.com/Shopify/quilt/pull/648)) diff --git a/packages/react-i18n/README.md b/packages/react-i18n/README.md index f3a210374d..499b5a6db4 100644 --- a/packages/react-i18n/README.md +++ b/packages/react-i18n/README.md @@ -69,6 +69,8 @@ export default function NotFound() { The hook also returns a `ShareTranslations` component. You can wrap this around a part of the subtree that should have access to this component’s translations. +> **Note:** `ShareTranslations` is not guaranteed to re-render when your i18n object changes. If you render `ShareTranslations` inside of a component that might block changes to children, you will likely run into issues. To prevent this, we recommend that `ShareTranslations` should be rendered as a top-level child of the component that uses `useI18n`. + ```tsx import * as React from 'react'; import {Page} from '@shopify/polaris'; @@ -81,13 +83,9 @@ interface Props { export default function ProductDetails({children}: Props) { const [i18n, ShareTranslations] = useI18n(); return ( - - - {children} - - + + {children} + ); } ``` @@ -120,26 +118,6 @@ class NotFound extends React.Component { export default withI18n()(NotFound); ``` -If you only need access to parent translations and/ or the various formatting utilities found on the `I18n` object, you can instead use the `useSimpleI18n` hook. This hook does not support providing any internationalization details for the component itself, but is a very performant way to access i18n utilities that are tied to the global locale. - -```tsx -import * as React from 'react'; -import {EmptyState} from '@shopify/polaris'; -import {useSimpleI18n} from '@shopify/react-i18n'; - -export default function NotFound() { - const i18n = useSimpleI18n(); - return ( - -

{i18n.translate('NotFound.content')}

- - ); -} -``` - #### `i18n` The provided `i18n` object exposes many useful methods for internationalizing your apps. You can see the full details in the [`i18n` source file](https://github.com/Shopify/quilt/blob/master/packages/react-i18n/src/i18n.ts), but you will commonly need the following: diff --git a/packages/react-i18n/package.json b/packages/react-i18n/package.json index b2c1b9b824..64c72a84b3 100644 --- a/packages/react-i18n/package.json +++ b/packages/react-i18n/package.json @@ -35,6 +35,7 @@ "dependencies": { "@shopify/i18n": "^0.1.3", "@shopify/react-effect": "^3.0.1", + "@shopify/react-hooks": "^1.0.0", "@types/hoist-non-react-statics": "^3.0.1", "hoist-non-react-statics": "^3.0.1", "prop-types": "^15.6.2", diff --git a/packages/react-i18n/src/context.ts b/packages/react-i18n/src/context.ts index 7561eeaf85..e3f47081f8 100644 --- a/packages/react-i18n/src/context.ts +++ b/packages/react-i18n/src/context.ts @@ -3,4 +3,5 @@ import {I18nManager} from './manager'; import {I18n} from './i18n'; export const I18nContext = React.createContext(null); -export const I18nParentsContext = React.createContext(null); +export const I18nIdsContext = React.createContext([]); +export const I18nParentContext = React.createContext(null); diff --git a/packages/react-i18n/src/hooks.tsx b/packages/react-i18n/src/hooks.tsx index 73563b4c7c..8c4bbd0a91 100644 --- a/packages/react-i18n/src/hooks.tsx +++ b/packages/react-i18n/src/hooks.tsx @@ -1,17 +1,14 @@ import * as React from 'react'; +import {useLazyRef} from '@shopify/react-hooks'; + import {I18n} from './i18n'; -import {RegisterOptions} from './manager'; -import {I18nContext, I18nParentsContext} from './context'; +import {I18nManager, RegisterOptions} from './manager'; +import {I18nContext, I18nIdsContext, I18nParentContext} from './context'; -export function useI18n({ - id, - fallback, - translations, -}: Partial = {}): [ - I18n, - React.ComponentType<{children: React.ReactNode}> -] { +type Result = [I18n, React.ComponentType<{children: React.ReactNode}>]; + +export function useI18n(options?: Partial): Result { const manager = React.useContext(I18nContext); if (manager == null) { @@ -20,59 +17,98 @@ export function useI18n({ ); } - const parentI18n = React.useContext(I18nParentsContext); - const parentIds = parentI18n ? parentI18n.ids || [] : []; - // eslint-disable-next-line react-hooks/exhaustive-deps - const ids = React.useMemo(() => (id ? [id, ...parentIds] : parentIds), [ - id, - // eslint-disable-next-line react-hooks/exhaustive-deps - ...parentIds, - ]); + const registerOptions = React.useRef(options); + + if (shouldRegister(registerOptions.current) !== shouldRegister(options)) { + throw new Error( + 'You switched between providing registration options and not providing them, which is not supported.', + ); + } + + // Yes, this would usually be dangerous. But just above this line, we check to make + // sure that they never switch from the case where `options == null` to `options != null`, + // so we know that a given use of this hook will only ever hit one of these two cases. + /* eslint-disable react-hooks/rules-of-hooks */ + if (options == null) { + return useSimpleI18n(manager); + } else { + return useComplexI18n(options, manager); + } + /* eslint-enable react-hooks/rules-of-hooks */ +} + +function useComplexI18n( + {id, fallback, translations}: Partial, + manager: I18nManager, +): Result { + const parentIds = React.useContext(I18nIdsContext); + + // Parent IDs can only change when a parent gets added/ removed, + // which would cause the component using `useI18n` to unmount. + // We also don't support the `id` changing between renders. For these + // reasons, it's safe to just store the IDs once and never let them change. + const ids = useLazyRef(() => (id ? [id, ...parentIds] : parentIds)); if (id && (translations || fallback)) { manager.register({id, translations, fallback}); } const [i18n, setI18n] = React.useState(() => { - const {translations} = manager.state(ids); - return new I18n(translations, manager.details, ids); + const {translations} = manager.state(ids.current); + return new I18n(translations, manager.details); }); + const i18nRef = React.useRef(i18n); + React.useEffect( () => { - return manager.subscribe(ids, ({translations}, details) => { - setI18n(new I18n(translations, details, ids)); + return manager.subscribe(ids.current, ({translations}, details) => { + const newI18n = new I18n(translations, details); + i18nRef.current = newI18n; + setI18n(newI18n); }); }, [ids, manager], ); - const ShareTranslations = React.useMemo( + // We use refs in this component so that it never changes. If this component + // is regenerated, it will unmount the entire tree of the previous component, + // which is usually not desirable. Technically, this does leave surface area + // for a bug to sneak in: if the component that renders this does so inside + // a component that blocks the update from passing down, nothing will force + // this component to re-render, so no descendants will get the new ids/ i18n + // value. Because we don't actually have any such cases, we're OK with this + // for now. + const shareTranslationsComponent = useLazyRef( () => function ShareTranslations({children}: {children: React.ReactNode}) { return ( - - {children} - + + + {children} + + ); }, - [i18n], ); - return [i18n, ShareTranslations]; + return [i18n, shareTranslationsComponent.current]; } -export function useSimpleI18n() { - const manager = React.useContext(I18nContext); +function useSimpleI18n(manager: I18nManager): Result { + const i18n = + React.useContext(I18nParentContext) || new I18n([], manager.details); - if (manager == null) { - throw new Error( - 'Missing i18n manager. Make sure to use an somewhere in your React tree.', - ); - } + return [i18n, IdentityComponent]; +} - const i18n = - React.useContext(I18nParentsContext) || new I18n([], manager.details); +function IdentityComponent({children}: {children: React.ReactNode}) { + return <>{children}; +} - return i18n; +function shouldRegister({ + fallback, + translations, +}: Partial = {}) { + return fallback != null || translations != null; } diff --git a/packages/react-i18n/src/i18n.ts b/packages/react-i18n/src/i18n.ts index 691ef573a4..8b8509ae31 100644 --- a/packages/react-i18n/src/i18n.ts +++ b/packages/react-i18n/src/i18n.ts @@ -102,7 +102,6 @@ export class I18n { pseudolocalize = false, onError, }: I18nDetails, - public readonly ids?: string[], ) { this.locale = locale; this.defaultCountry = country; diff --git a/packages/react-i18n/src/index.ts b/packages/react-i18n/src/index.ts index 7ce5caafcf..cabff07016 100644 --- a/packages/react-i18n/src/index.ts +++ b/packages/react-i18n/src/index.ts @@ -1,7 +1,7 @@ export {I18nManager, ExtractedTranslations} from './manager'; export {I18nContext} from './context'; export {I18n} from './i18n'; -export {useI18n, useSimpleI18n} from './hooks'; +export {useI18n} from './hooks'; export {withI18n, WithI18nProps} from './decorator'; export {translate} from './utilities'; export {I18nDetails, LanguageDirection, CurrencyCode} from './types'; diff --git a/packages/react-i18n/src/manager.ts b/packages/react-i18n/src/manager.ts index 3928c6b8d0..382d869cc4 100644 --- a/packages/react-i18n/src/manager.ts +++ b/packages/react-i18n/src/manager.ts @@ -51,6 +51,9 @@ export class I18nManager { private subscriptions = new Map(); private translationPromises = new Map>(); + private idsToUpdate = new Set(); + private enqueuedUpdate?: number; + constructor( public details: I18nDetails, initialTranslations: ExtractedTranslations = {}, @@ -106,6 +109,7 @@ export class I18nManager { fallbackLocale != null && possibleLocales.includes(fallbackLocale); let loading = false; + let hasUnresolvedTranslations = false; const translations = ids.reduce( (otherTranslations, id) => { @@ -114,15 +118,20 @@ export class I18nManager { for (const locale of possibleLocales) { const translationId = getTranslationId(id, locale); const translation = this.translations.get(translationId); + if (translation == null) { if (this.translationPromises.has(translationId)) { - loading = true; + hasUnresolvedTranslations = true; } } else { translationsForId.push(translation); } } + if (translationsForId.length === 0 && hasUnresolvedTranslations) { + loading = true; + } + if (!omitFallbacks) { const fallback = this.fallbacks.get(id); if (fallback != null) { @@ -175,13 +184,15 @@ export class I18nManager { this.translationPromises.delete(translationId); this.translations.set(translationId, result); this.asyncTranslationIds.add(translationId); - this.updateSubscribersForId(id); + + if (result != null) { + this.updateSubscribersForId(id); + } }) .catch(() => { this.translationPromises.delete(translationId); this.translations.set(translationId, undefined); this.asyncTranslationIds.add(translationId); - this.updateSubscribersForId(id); }); this.translationPromises.set(translationId, promise); @@ -192,11 +203,27 @@ export class I18nManager { } private updateSubscribersForId(id: string) { - for (const [subscriber, ids] of this.subscriptions) { - if (ids.includes(id)) { - subscriber(this.state(ids), this.details); - } + this.idsToUpdate.add(id); + + if (this.enqueuedUpdate != null) { + return; } + + const isBrowser = typeof window !== 'undefined'; + const enqueue = isBrowser ? window.requestAnimationFrame : setImmediate; + + this.enqueuedUpdate = enqueue(() => { + delete this.enqueuedUpdate; + + const idsToUpdate = [...this.idsToUpdate]; + this.idsToUpdate.clear(); + + for (const [subscriber, ids] of this.subscriptions) { + if (ids.some(id => idsToUpdate.includes(id))) { + subscriber(this.state(ids), this.details); + } + } + }); } } diff --git a/packages/react-i18n/src/test/manager.test.ts b/packages/react-i18n/src/test/manager.test.ts index 0023fa5e2c..a2e25392b4 100644 --- a/packages/react-i18n/src/test/manager.test.ts +++ b/packages/react-i18n/src/test/manager.test.ts @@ -1,8 +1,18 @@ import './matchers'; +import {animationFrame} from '@shopify/jest-dom-mocks'; + import {I18nManager} from '../manager'; describe('I18nManager', () => { + beforeEach(() => { + animationFrame.mock(); + }); + + afterEach(() => { + animationFrame.restore(); + }); + const basicDetails = {locale: 'en'}; const en = {hello: 'Hello'}; @@ -327,15 +337,17 @@ describe('I18nManager', () => { expect(spy).not.toHaveBeenCalled(); await frCATranslation.resolve(); + animationFrame.runFrame(); expect(spy).toHaveBeenCalledTimes(1); expect(spy).toHaveBeenCalledWith(manager.state([id]), manager.details); await frTranslation.resolve(); + animationFrame.runFrame(); expect(spy).toHaveBeenCalledTimes(2); expect(spy).toHaveBeenCalledWith(manager.state([id]), manager.details); }); - it('calls the subscription even when a translation rejects', async () => { + it('enqueues multiple translations resolving into the same frame', async () => { const id = createID(); const spy = jest.fn(); const frTranslation = createTranslationPromise(fr); @@ -357,11 +369,48 @@ describe('I18nManager', () => { }); manager.subscribe([id], spy); + await frCATranslation.resolve(); + await frTranslation.resolve(); + expect(spy).not.toHaveBeenCalled(); - await frCATranslation.reject(); + animationFrame.runFrame(); + expect(spy).toHaveBeenCalledTimes(1); - expect(spy).toHaveBeenCalledWith(manager.state([id]), manager.details); + }); + + it('does not call the subscription when a translation resolves to undefined', async () => { + const id = createID(); + const spy = jest.fn(); + const frTranslation = createTranslationPromise(undefined); + + const manager = new I18nManager({...basicDetails, locale: 'fr-CA'}); + + manager.register({ + id, + translations: () => frTranslation.promise, + }); + manager.subscribe([id], spy); + + await frTranslation.resolve(); + expect(spy).not.toHaveBeenCalled(); + }); + + it('does not call the subscription when a translation rejects', async () => { + const id = createID(); + const spy = jest.fn(); + const frTranslation = createTranslationPromise(fr); + + const manager = new I18nManager({...basicDetails, locale: 'fr-CA'}); + + manager.register({ + id, + translations: () => frTranslation.promise, + }); + manager.subscribe([id], spy); + + await frTranslation.reject(); + expect(spy).not.toHaveBeenCalled(); }); }); }); diff --git a/yarn.lock b/yarn.lock index a93b29d8e5..22b00aea6e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7080,6 +7080,18 @@ rc@^1.0.1, rc@^1.1.6, rc@^1.1.7: minimist "^1.2.0" strip-json-comments "~2.0.1" +"react-apollo@>=2.2.3 <3.0.0": + version "2.5.4" + resolved "https://registry.yarnpkg.com/react-apollo/-/react-apollo-2.5.4.tgz#4b5ad2e9e38001521d072445acdb5b3b5489b22d" + integrity sha512-olH9zYijOXVfj14hD7bQlZ0POBJchxg2e+mfnxEiEdqZra4+58SfIY0KPhmM9jqbeeusc6J/P4zzWIHt5DdNDg== + dependencies: + apollo-utilities "^1.2.1" + hoist-non-react-statics "^3.3.0" + lodash.isequal "^4.5.0" + prop-types "^15.7.2" + ts-invariant "^0.3.2" + tslib "^1.9.3" + react-apollo@^2.2.3: version "2.5.2" resolved "https://registry.yarnpkg.com/react-apollo/-/react-apollo-2.5.2.tgz#6732c6af55e6adc9ebf97bf189e867a893c449d3" @@ -7092,18 +7104,6 @@ react-apollo@^2.2.3: ts-invariant "^0.3.0" tslib "^1.9.3" -react-apollo@~2.5.3: - version "2.5.4" - resolved "https://registry.yarnpkg.com/react-apollo/-/react-apollo-2.5.4.tgz#4b5ad2e9e38001521d072445acdb5b3b5489b22d" - integrity sha512-olH9zYijOXVfj14hD7bQlZ0POBJchxg2e+mfnxEiEdqZra4+58SfIY0KPhmM9jqbeeusc6J/P4zzWIHt5DdNDg== - dependencies: - apollo-utilities "^1.2.1" - hoist-non-react-statics "^3.3.0" - lodash.isequal "^4.5.0" - prop-types "^15.7.2" - ts-invariant "^0.3.2" - tslib "^1.9.3" - react-dom@^16.8.1: version "16.8.1" resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-16.8.1.tgz#ec860f98853d09d39bafd3a6f1e12389d283dbb4"