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"