From 7fc4945c05415bbcaf082aa3e7c79662f6b15b69 Mon Sep 17 00:00:00 2001 From: Chris Sauve Date: Wed, 17 Apr 2019 09:20:37 -0400 Subject: [PATCH 1/6] i18n performance optimizations --- packages/react-i18n/README.md | 20 ---------- packages/react-i18n/src/context.ts | 3 +- packages/react-i18n/src/hooks.tsx | 62 +++++++++++++++++++----------- packages/react-i18n/src/index.ts | 2 +- packages/react-i18n/src/manager.ts | 29 ++++++++++++-- 5 files changed, 67 insertions(+), 49 deletions(-) diff --git a/packages/react-i18n/README.md b/packages/react-i18n/README.md index f3a210374d..430f08b18d 100644 --- a/packages/react-i18n/README.md +++ b/packages/react-i18n/README.md @@ -120,26 +120,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/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..c281913868 100644 --- a/packages/react-i18n/src/hooks.tsx +++ b/packages/react-i18n/src/hooks.tsx @@ -1,14 +1,10 @@ import * as React from 'react'; 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 = {}): [ +export function useI18n(options?: Partial): [ I18n, React.ComponentType<{children: React.ReactNode}> ] { @@ -20,8 +16,26 @@ export function useI18n({ ); } - const parentI18n = React.useContext(I18nParentsContext); - const parentIds = parentI18n ? parentI18n.ids || [] : []; + 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.'); + } + + if (options == null) { + return useSimpleI18n(manager); + } else { + return useComplexI18n(options!, manager); + } +} + +function useComplexI18n({ + id, + fallback, + translations, +}: Partial, manager: I18nManager) { + const parentIds = React.useContext(I18nIdsContext); + // eslint-disable-next-line react-hooks/exhaustive-deps const ids = React.useMemo(() => (id ? [id, ...parentIds] : parentIds), [ id, @@ -51,28 +65,30 @@ export function useI18n({ () => function ShareTranslations({children}: {children: React.ReactNode}) { return ( - - {children} - + + + {children} + + ); }, - [i18n], + [i18n, ids], ); return [i18n, ShareTranslations]; } -export function useSimpleI18n() { - const manager = React.useContext(I18nContext); +function useSimpleI18n(manager: I18nManager) { + 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/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..ce9ed4c56c 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 = {}, @@ -192,11 +195,29 @@ 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 enqueuer = isBrowser ? window.requestAnimationFrame : setImmediate; + const dequeuer = isBrowser ? window.cancelAnimationFrame : clearImmediate; + + this.enqueuedUpdate = enqueuer(() => { + dequeuer(this.enqueuedUpdate!); + + const {idsToUpdate} = this; + + for (const [subscriber, ids] of this.subscriptions) { + if (ids.some(id => idsToUpdate.has(id))) { + subscriber(this.state(ids), this.details); + } + } + + idsToUpdate.clear(); + }); } } From fdeb880b71247af070a87d76908cc975a7d8c86c Mon Sep 17 00:00:00 2001 From: Chris Sauve Date: Wed, 17 Apr 2019 11:16:54 -0400 Subject: [PATCH 2/6] I18n perf improvements --- 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 | 2 + packages/react-i18n/package.json | 1 + packages/react-i18n/src/hooks.tsx | 70 ++++++++++++------- packages/react-i18n/src/manager.ts | 28 +++++--- packages/react-i18n/src/test/manager.test.ts | 55 ++++++++++++++- yarn.lock | 12 +++- 9 files changed, 176 insertions(+), 43 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/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..77dcc9e874 100644 --- a/packages/react-i18n/CHANGELOG.md +++ b/packages/react-i18n/CHANGELOG.md @@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ## [Unreleased] +## [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/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/hooks.tsx b/packages/react-i18n/src/hooks.tsx index c281913868..e4d46ce46a 100644 --- a/packages/react-i18n/src/hooks.tsx +++ b/packages/react-i18n/src/hooks.tsx @@ -1,13 +1,14 @@ import * as React from 'react'; +import {useLazyRef} from '@shopify/react-hooks'; + import {I18n} from './i18n'; import {I18nManager, RegisterOptions} from './manager'; import {I18nContext, I18nIdsContext, I18nParentContext} from './context'; -export function useI18n(options?: 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) { @@ -19,7 +20,9 @@ export function useI18n(options?: Partial): [ 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.'); + throw new Error( + 'You switched between providing registration options and not providing them, which is not supported.', + ); } if (options == null) { @@ -29,56 +32,66 @@ export function useI18n(options?: Partial): [ } } -function useComplexI18n({ - id, - fallback, - translations, -}: Partial, manager: I18nManager) { +function useComplexI18n( + {id, fallback, translations}: Partial, + manager: I18nManager, +): Result { const parentIds = React.useContext(I18nIdsContext); - // 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, - ]); + // 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, ids.current); }); + 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, ids.current); + i18nRef.current = newI18n; + setI18n(newI18n); }); }, - [ids, manager], + [manager], ); + // 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 ShareTranslations = React.useMemo( () => function ShareTranslations({children}: {children: React.ReactNode}) { return ( - - + + {children} ); }, - [i18n, ids], + [i18nRef, ids], ); return [i18n, ShareTranslations]; } -function useSimpleI18n(manager: I18nManager) { +function useSimpleI18n(manager: I18nManager): Result { const i18n = React.useContext(I18nParentContext) || new I18n([], manager.details); @@ -86,9 +99,12 @@ function useSimpleI18n(manager: I18nManager) { } function IdentityComponent({children}: {children: React.ReactNode}) { - return children; + return <>{children}; } -function shouldRegister({fallback, translations}: Partial = {}) { +function shouldRegister({ + fallback, + translations, +}: Partial = {}) { return fallback != null || translations != null; } diff --git a/packages/react-i18n/src/manager.ts b/packages/react-i18n/src/manager.ts index ce9ed4c56c..382d869cc4 100644 --- a/packages/react-i18n/src/manager.ts +++ b/packages/react-i18n/src/manager.ts @@ -109,6 +109,7 @@ export class I18nManager { fallbackLocale != null && possibleLocales.includes(fallbackLocale); let loading = false; + let hasUnresolvedTranslations = false; const translations = ids.reduce( (otherTranslations, id) => { @@ -117,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) { @@ -178,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); @@ -202,21 +210,19 @@ export class I18nManager { } const isBrowser = typeof window !== 'undefined'; - const enqueuer = isBrowser ? window.requestAnimationFrame : setImmediate; - const dequeuer = isBrowser ? window.cancelAnimationFrame : clearImmediate; + const enqueue = isBrowser ? window.requestAnimationFrame : setImmediate; - this.enqueuedUpdate = enqueuer(() => { - dequeuer(this.enqueuedUpdate!); + this.enqueuedUpdate = enqueue(() => { + delete this.enqueuedUpdate; - const {idsToUpdate} = this; + const idsToUpdate = [...this.idsToUpdate]; + this.idsToUpdate.clear(); for (const [subscriber, ids] of this.subscriptions) { - if (ids.some(id => idsToUpdate.has(id))) { + if (ids.some(id => idsToUpdate.includes(id))) { subscriber(this.state(ids), this.details); } } - - idsToUpdate.clear(); }); } } 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..ac9ea8c778 100644 --- a/yarn.lock +++ b/yarn.lock @@ -550,7 +550,7 @@ resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.5.8.tgz#8ae4e0ea205fe95c3901a5a1df7f66495e3a56ce" integrity sha512-3AQoUxQcQtLHsK25wtTWIoIpgYjH3vSDroZOUr7PpCHw/jLY1RB9z9E8dBT/OSmwStVgkRNvdh+ZHNiomRieaw== -"@types/react-dom@16.8.3", "@types/react-dom@^16.0.11", "@types/react-dom@^16.8.3": +"@types/react-dom@^16.0.11", "@types/react-dom@^16.8.3": version "16.8.3" resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-16.8.3.tgz#6131b7b6158bc7ed1925a3374b88b7c00481f0cb" integrity sha512-HF5hD5YR3z9Mn6kXcW1VKe4AQ04ZlZj1EdLBae61hzQ3eEWWxMgNLUbIxeZp40BnSxqY1eAYLsH9QopQcxzScA== @@ -572,7 +572,7 @@ "@types/history" "^3" "@types/react" "*" -"@types/react@*", "@types/react@16.8.10", "@types/react@16.8.2", "@types/react@>=16.4.0", "@types/react@^16.0.2": +"@types/react@*", "@types/react@>=16.4.0", "@types/react@^16.0.2": version "16.8.10" resolved "https://registry.yarnpkg.com/@types/react/-/react-16.8.10.tgz#1ccb6fde17f71a62ef055382ec68bdc379d4d8d9" integrity sha512-7bUQeZKP4XZH/aB4i7k1i5yuwymDu/hnLMhD9NjVZvQQH7ZUgRN3d6iu8YXzx4sN/tNr0bj8jgguk8hhObzGvA== @@ -580,6 +580,14 @@ "@types/prop-types" "*" csstype "^2.2.0" +"@types/react@16.8.2": + version "16.8.2" + resolved "https://registry.yarnpkg.com/@types/react/-/react-16.8.2.tgz#3b7a7f7ea89d1c7d68b00849fb5de839011c077b" + integrity sha512-6mcKsqlqkN9xADrwiUz2gm9Wg4iGnlVGciwBRYFQSMWG6MQjhOZ/AVnxn+6v8nslFgfYTV8fNdE6XwKu6va5PA== + dependencies: + "@types/prop-types" "*" + csstype "^2.2.0" + "@types/safe-compare@^1.1.0": version "1.1.0" resolved "https://registry.yarnpkg.com/@types/safe-compare/-/safe-compare-1.1.0.tgz#47ed9b9ca51a3a791b431cd59b28f47fa9bf1224" From 174510709c940e0647871045f12ba28d367db426 Mon Sep 17 00:00:00 2001 From: Chris Sauve Date: Wed, 17 Apr 2019 11:31:55 -0400 Subject: [PATCH 3/6] Document updates --- packages/react-hooks/CHANGELOG.md | 8 +++++++- packages/react-hooks/README.md | 16 ++++++++++++++++ packages/react-i18n/CHANGELOG.md | 4 ++++ 3 files changed, 27 insertions(+), 1 deletion(-) 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-i18n/CHANGELOG.md b/packages/react-i18n/CHANGELOG.md index 77dcc9e874..f641188730 100644 --- a/packages/react-i18n/CHANGELOG.md +++ b/packages/react-i18n/CHANGELOG.md @@ -8,6 +8,10 @@ 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 From d7cd08e594800f7e78ed92cf085ff37273589350 Mon Sep 17 00:00:00 2001 From: Chris Sauve Date: Wed, 17 Apr 2019 11:59:34 -0400 Subject: [PATCH 4/6] Lint fixes --- packages/react-i18n/src/hooks.tsx | 18 +++++++++++------- packages/react-i18n/src/i18n.ts | 1 - 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/react-i18n/src/hooks.tsx b/packages/react-i18n/src/hooks.tsx index e4d46ce46a..8c4bbd0a91 100644 --- a/packages/react-i18n/src/hooks.tsx +++ b/packages/react-i18n/src/hooks.tsx @@ -25,11 +25,16 @@ export function useI18n(options?: Partial): Result { ); } + // 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); + return useComplexI18n(options, manager); } + /* eslint-enable react-hooks/rules-of-hooks */ } function useComplexI18n( @@ -50,7 +55,7 @@ function useComplexI18n( const [i18n, setI18n] = React.useState(() => { const {translations} = manager.state(ids.current); - return new I18n(translations, manager.details, ids.current); + return new I18n(translations, manager.details); }); const i18nRef = React.useRef(i18n); @@ -58,12 +63,12 @@ function useComplexI18n( React.useEffect( () => { return manager.subscribe(ids.current, ({translations}, details) => { - const newI18n = new I18n(translations, details, ids.current); + const newI18n = new I18n(translations, details); i18nRef.current = newI18n; setI18n(newI18n); }); }, - [manager], + [ids, manager], ); // We use refs in this component so that it never changes. If this component @@ -74,7 +79,7 @@ function useComplexI18n( // 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 ShareTranslations = React.useMemo( + const shareTranslationsComponent = useLazyRef( () => function ShareTranslations({children}: {children: React.ReactNode}) { return ( @@ -85,10 +90,9 @@ function useComplexI18n(
); }, - [i18nRef, ids], ); - return [i18n, ShareTranslations]; + return [i18n, shareTranslationsComponent.current]; } function useSimpleI18n(manager: I18nManager): Result { 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; From cc34834499eddc22c04883d6ac0c34e41b763781 Mon Sep 17 00:00:00 2001 From: Chris Sauve Date: Wed, 17 Apr 2019 13:41:24 -0400 Subject: [PATCH 5/6] Make note of ShareTranslations on README --- packages/react-i18n/README.md | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/react-i18n/README.md b/packages/react-i18n/README.md index 430f08b18d..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} + ); } ``` From 05887cdee863da83be75b50568f0202d1f5cc789 Mon Sep 17 00:00:00 2001 From: Chris Sauve Date: Wed, 17 Apr 2019 13:50:14 -0400 Subject: [PATCH 6/6] Fix out-of-date lockfile --- yarn.lock | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/yarn.lock b/yarn.lock index ac9ea8c778..22b00aea6e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -550,7 +550,7 @@ resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.5.8.tgz#8ae4e0ea205fe95c3901a5a1df7f66495e3a56ce" integrity sha512-3AQoUxQcQtLHsK25wtTWIoIpgYjH3vSDroZOUr7PpCHw/jLY1RB9z9E8dBT/OSmwStVgkRNvdh+ZHNiomRieaw== -"@types/react-dom@^16.0.11", "@types/react-dom@^16.8.3": +"@types/react-dom@16.8.3", "@types/react-dom@^16.0.11", "@types/react-dom@^16.8.3": version "16.8.3" resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-16.8.3.tgz#6131b7b6158bc7ed1925a3374b88b7c00481f0cb" integrity sha512-HF5hD5YR3z9Mn6kXcW1VKe4AQ04ZlZj1EdLBae61hzQ3eEWWxMgNLUbIxeZp40BnSxqY1eAYLsH9QopQcxzScA== @@ -572,7 +572,7 @@ "@types/history" "^3" "@types/react" "*" -"@types/react@*", "@types/react@>=16.4.0", "@types/react@^16.0.2": +"@types/react@*", "@types/react@16.8.10", "@types/react@16.8.2", "@types/react@>=16.4.0", "@types/react@^16.0.2": version "16.8.10" resolved "https://registry.yarnpkg.com/@types/react/-/react-16.8.10.tgz#1ccb6fde17f71a62ef055382ec68bdc379d4d8d9" integrity sha512-7bUQeZKP4XZH/aB4i7k1i5yuwymDu/hnLMhD9NjVZvQQH7ZUgRN3d6iu8YXzx4sN/tNr0bj8jgguk8hhObzGvA== @@ -580,14 +580,6 @@ "@types/prop-types" "*" csstype "^2.2.0" -"@types/react@16.8.2": - version "16.8.2" - resolved "https://registry.yarnpkg.com/@types/react/-/react-16.8.2.tgz#3b7a7f7ea89d1c7d68b00849fb5de839011c077b" - integrity sha512-6mcKsqlqkN9xADrwiUz2gm9Wg4iGnlVGciwBRYFQSMWG6MQjhOZ/AVnxn+6v8nslFgfYTV8fNdE6XwKu6va5PA== - dependencies: - "@types/prop-types" "*" - csstype "^2.2.0" - "@types/safe-compare@^1.1.0": version "1.1.0" resolved "https://registry.yarnpkg.com/@types/safe-compare/-/safe-compare-1.1.0.tgz#47ed9b9ca51a3a791b431cd59b28f47fa9bf1224" @@ -7088,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" @@ -7100,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"