react-i18n perf optimizations (#659)

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
 

packages/react-hooks/README.md

@@ -40,3 +40,19 @@ function MyComponent() {
   return <div>{foo}</div>;
 }
 ```
+
+### `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;
+}
+```

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';

packages/react-hooks/src/hooks/lazy-ref.ts

@@ -0,0 +1,13 @@
+import {useRef, MutableRefObject} from 'react';
+
+const UNSET = Symbol('unset');
+
+export function useLazyRef<T>(getValue: () => T): MutableRefObject<T> {
+  const ref = useRef<T | typeof UNSET>(UNSET);
+
+  if (ref.current === UNSET) {
+    ref.current = getValue();
+  }
+
+  return ref as MutableRefObject<T>;
+}

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 <div>{value.current}</div>;
+    }
+
+    const mockComponent = mount(<MockComponent />);
+
+    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 <div>{value.current}</div>;
+    }
+
+    const mockComponent = mount(<MockComponent spy={() => 'Hello'} />);
+
+    const newContent = 'Goodbye';
+    const spy = jest.fn(() => newContent);
+    mockComponent.setProps({spy});
+
+    expect(spy).not.toHaveBeenCalled();
+    expect(mockComponent).not.toContainReactText(newContent);
+  });
+});

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

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 (
-    <Page
-      title={i18n.translate('ProductDetails.title')}
-    >
-      <ShareTranslations>
-        {children}
-      </ShareTranslations>
-    </EmptyState>
+    <ShareTranslations>
+      <Page title={i18n.translate('ProductDetails.title')}>{children}</Page>
+    </ShareTranslations>
   );
 }
 ```
@@ -120,26 +118,6 @@ class NotFound extends React.Component<ComposedProps> {
 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 (
-    <EmptyState
-      heading={i18n.translate('NotFound.heading')}
-      action={{content: i18n.translate('Common.back'), url: '/'}}
-    >
-      <p>{i18n.translate('NotFound.content')}</p>
-    </EmptyState>
-  );
-}
-```
-
 #### `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:

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",

packages/react-i18n/src/context.ts

@@ -3,4 +3,5 @@ import {I18nManager} from './manager';
 import {I18n} from './i18n';
 
 export const I18nContext = React.createContext<I18nManager | null>(null);
-export const I18nParentsContext = React.createContext<I18n | null>(null);
+export const I18nIdsContext = React.createContext<string[]>([]);
+export const I18nParentContext = React.createContext<I18n | null>(null);

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<RegisterOptions> = {}): [
-  I18n,
-  React.ComponentType<{children: React.ReactNode}>
-] {
+type Result = [I18n, React.ComponentType<{children: React.ReactNode}>];
+
+export function useI18n(options?: Partial<RegisterOptions>): 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<RegisterOptions>,
+  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 (
-          <I18nParentsContext.Provider value={i18n}>
-            {children}
-          </I18nParentsContext.Provider>
+          <I18nIdsContext.Provider value={ids.current}>
+            <I18nParentContext.Provider value={i18nRef.current}>
+              {children}
+            </I18nParentContext.Provider>
+          </I18nIdsContext.Provider>
         );
       },
-    [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 <I18nContext.Provider /> 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<RegisterOptions> = {}) {
+  return fallback != null || translations != null;
 }

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;

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';