From b751622f3f1166d2554f14b32a419294caa5e985 Mon Sep 17 00:00:00 2001 From: kamikazePT Date: Sat, 5 Sep 2020 01:09:22 +0100 Subject: [PATCH] fix: Fixed lazy usage with Suspense and Error Boundary together (#521) * Fixed lazy usage with Suspense and Error Boundary together * Typo * v1.0.0-fork * accidental push * Condition was reversed * Fixed Suspense with full dynamic import after fulfilled promise * Added unit test * cached promise * added tests for multiple elements of same async component * renamed unit test * added retryable error boundary * reworked tests * Retrying working for lazy and loadable * linter should run on pre-commit... :/ * upped max bundle size * fix: Fixed suspense tests and fixed un-throwable pending promises when using suspense * refactor: removed unnecessary wait in test * test: fixed test regarding nested suspense * test: fixed fucked up assertion * fix: fixed weird corner case on error boundary not being reached * feat: removed proxy * fix: removed unnecessary code * fix: fixed unnecesssary stack of callbacks --- src/createLoadable.js | 87 +++++++++------- src/library.js | 4 +- src/loadable.test.js | 231 +++++++++++++++++++++++++++++++++--------- 3 files changed, 237 insertions(+), 85 deletions(-) diff --git a/src/createLoadable.js b/src/createLoadable.js index dd22c647..d77c0bfa 100644 --- a/src/createLoadable.js +++ b/src/createLoadable.js @@ -6,6 +6,10 @@ import { invariant } from './util' import Context from './Context' import { LOADABLE_SHARED } from './shared' +const STATUS_PENDING = 'PENDING' +const STATUS_RESOLVED = 'RESOLVED' +const STATUS_REJECTED = 'REJECTED' + function resolveConstructor(ctor) { if (typeof ctor === 'function') { return { requireAsync: ctor } @@ -75,8 +79,6 @@ function createLoadable({ cacheKey: getCacheKey(props), } - this.promise = null - invariant( !props.__chunkExtractor || ctor.requireSync, 'SSR requires `@loadable/babel-plugin`, please install it', @@ -119,18 +121,22 @@ function createLoadable({ componentDidMount() { this.mounted = true - if (this.state.loading) { - this.loadAsync() - } else if (!this.state.error) { - this.triggerOnLoad() + const cachedPromise = this.getCache() + + if (cachedPromise && cachedPromise.status === STATUS_REJECTED) { + this.setCache() + this.setState({ + error: undefined, + loading: true, + }) } + this.loadAsyncOnLifecycle() } componentDidUpdate(prevProps, prevState) { // Component is reloaded if the cacheKey has changed if (prevState.cacheKey !== this.state.cacheKey) { - this.promise = null - this.loadAsync() + this.loadAsyncOnLifecycle() } } @@ -178,29 +184,45 @@ function createLoadable({ } loadAsync() { - if (!this.promise) { - const { __chunkExtractor, forwardedRef, ...props } = this.props - this.promise = ctor - .requireAsync(props) + const { __chunkExtractor, forwardedRef, ...props } = this.props + + let promise = this.getCache() + + if (!promise) { + promise = ctor.requireAsync(props) + promise.status = STATUS_PENDING + + this.setCache(promise) + + const cachedPromise = promise + + promise = promise .then(loadedModule => { - const result = resolve(loadedModule, this.props, Loadable) - if (options.suspense) { - this.setCache(result) - } - this.safeSetState( - { - result: resolve(loadedModule, this.props, Loadable), - loading: false, - }, - () => this.triggerOnLoad(), - ) + cachedPromise.status = STATUS_RESOLVED + return loadedModule }) .catch(error => { - this.safeSetState({ error, loading: false }) + cachedPromise.status = STATUS_REJECTED + throw error }) } - return this.promise + return promise + } + + loadAsyncOnLifecycle() { + this.loadAsync() + .then(loadedModule => { + const result = resolve(loadedModule, this.props, { Loadable }) + this.safeSetState( + { + result, + loading: false, + }, + () => this.triggerOnLoad(), + ) + }) + .catch(error => this.safeSetState({ error, loading: false })) } render() { @@ -213,15 +235,10 @@ function createLoadable({ const { error, loading, result } = this.state if (options.suspense) { - const cachedResult = this.getCache() - if (!cachedResult) throw this.loadAsync() - return render({ - loading: false, - fallback: null, - result: cachedResult, - options, - props: { ...props, ref: forwardedRef }, - }) + const cachedPromise = this.getCache() + if (!cachedPromise || cachedPromise.status === STATUS_PENDING) { + throw this.loadAsync() + } } if (error) { @@ -235,8 +252,6 @@ function createLoadable({ } return render({ - loading, - fallback, result, options, props: { ...props, ref: forwardedRef }, diff --git a/src/library.js b/src/library.js index b7e82dec..8d945dc6 100644 --- a/src/library.js +++ b/src/library.js @@ -11,8 +11,8 @@ export const { loadable, lazy } = createLoadable({ } } }, - render({ result, loading, props }) { - if (!loading && props.children) { + render({ result, props }) { + if (props.children) { return props.children(result) } diff --git a/src/loadable.test.js b/src/loadable.test.js index e400f150..b3691062 100644 --- a/src/loadable.test.js +++ b/src/loadable.test.js @@ -1,3 +1,4 @@ +/* eslint-disable max-classes-per-file */ /* eslint-disable import/no-extraneous-dependencies, react/no-multi-comp */ import 'regenerator-runtime/runtime' import '@testing-library/jest-dom/extend-expect' @@ -7,14 +8,13 @@ import loadable, { lazy } from './index' afterEach(cleanup) -function createLoadFunction() { - const ref = {} - const fn = jest.fn(() => ref.promise) - ref.promise = new Promise((resolve, reject) => { - fn.resolve = resolve - fn.reject = reject - }) - return fn +const unresolvableLoad = jest.fn(() => new Promise(() => {})) + +function mockDelayedResolvedValueOnce(resolvedValue) { + return this.mockImplementationOnce( + () => + new Promise(resolve => setTimeout(() => resolve(resolvedValue), 1000)), + ) } class Catch extends React.Component { @@ -29,6 +29,35 @@ class Catch extends React.Component { } } +class ErrorBoundary extends React.Component { + constructor(props) { + super(props) + + this.state = { + error: false, + retries: props.retries || 0, + } + } + + componentDidCatch() { + this.setState(prevState => ({ + error: true, + retries: prevState.retries - 1, + })) + } + + render() { + const { children, fallback } = this.props + const { error, retries } = this.state + + if (error) { + return (retries >= 0 && children) || fallback || null + } + + return children || null + } +} + describe('#loadable', () => { beforeEach(() => { jest.spyOn(console, 'error').mockImplementation(() => {}) @@ -40,44 +69,39 @@ describe('#loadable', () => { }) it('renders nothing without a fallback', () => { - const load = createLoadFunction() - const Component = loadable(load) + const Component = loadable(unresolvableLoad) const { container } = render() expect(container).toBeEmpty() }) it('uses option fallback if specified', () => { - const load = createLoadFunction() - const Component = loadable(load, { fallback: 'progress' }) + const Component = loadable(unresolvableLoad, { fallback: 'progress' }) const { container } = render() expect(container).toHaveTextContent('progress') }) it('uses props fallback if specified', () => { - const load = createLoadFunction() - const Component = loadable(load) + const Component = loadable(unresolvableLoad) const { container } = render() expect(container).toHaveTextContent('progress') }) it('should use props fallback instead of option fallback if specified', () => { - const load = createLoadFunction() - const Component = loadable(load, { fallback: 'opt fallback' }) + const Component = loadable(unresolvableLoad, { fallback: 'opt fallback' }) const { container } = render() expect(container).toHaveTextContent('prop fallback') }) it('mounts component when loaded', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ default: () => 'loaded' }) const Component = loadable(load) const { container } = render() expect(container).toBeEmpty() - load.resolve({ default: () => 'loaded' }) await wait(() => expect(container).toHaveTextContent('loaded')) }) it('supports preload', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ default: () => 'loaded' }) const Component = loadable(load) expect(load).not.toHaveBeenCalled() Component.preload({ foo: 'bar' }) @@ -85,28 +109,26 @@ describe('#loadable', () => { expect(load).toHaveBeenCalledTimes(1) const { container } = render() expect(container).toBeEmpty() - load.resolve({ default: () => 'loaded' }) await wait(() => expect(container).toHaveTextContent('loaded')) expect(load).toHaveBeenCalledTimes(2) }) it('supports commonjs default export', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue(() => 'loaded') const Component = loadable(load) const { container } = render() - load.resolve(() => 'loaded') await wait(() => expect(container).toHaveTextContent('loaded')) }) it('supports non-default export via resolveComponent', async () => { - const load = createLoadFunction() const importedModule = { exported: () => 'loaded' } + const load = jest.fn().mockResolvedValue(importedModule) const resolveComponent = jest.fn(({ exported: component }) => component) const Component = loadable(load, { resolveComponent, }) const { container } = render() - load.resolve(importedModule) + await wait(() => expect(container).toHaveTextContent('loaded')) expect(resolveComponent).toHaveBeenCalledWith(importedModule, { someProp: '123', @@ -116,18 +138,16 @@ describe('#loadable', () => { }) it('forwards props', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ default: ({ name }) => name }) const Component = loadable(load) const { container } = render() - load.resolve({ default: ({ name }) => name }) await wait(() => expect(container).toHaveTextContent('James Bond')) }) it('should update component if props change', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ default: ({ value }) => value }) const Component = loadable(load) const { container } = render() - load.resolve({ default: ({ value }) => value }) await wait(() => expect(container).toHaveTextContent('first')) render(, { container }) await wait(() => expect(container).toHaveTextContent('second')) @@ -135,10 +155,9 @@ describe('#loadable', () => { }) it('calls load func if cacheKey change', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ default: ({ value }) => value }) const Component = loadable(load, { cacheKey: ({ value }) => value }) const { container } = render() - load.resolve({ default: ({ value }) => value }) await wait(() => expect(container).toHaveTextContent('first')) expect(load).toHaveBeenCalledTimes(1) render(, { container }) @@ -147,13 +166,12 @@ describe('#loadable', () => { }) it('calls load func if resolve change', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ default: ({ value }) => value }) const Component = loadable({ requireAsync: load, resolve: ({ value }) => value, }) const { container } = render() - load.resolve({ default: ({ value }) => value }) await wait(() => expect(container).toHaveTextContent('first')) expect(load).toHaveBeenCalledTimes(1) render(, { container }) @@ -192,18 +210,17 @@ describe('#loadable', () => { }) it('forwards ref', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ + default: React.forwardRef((props, fref) =>
), + }) const Component = loadable(load) const ref = React.createRef() render() - load.resolve({ - default: React.forwardRef((props, fref) =>
), - }) await wait(() => expect(ref.current.tagName).toBe('DIV')) }) it('throws when an error occurs', async () => { - const load = createLoadFunction() + const load = jest.fn().mockRejectedValue(new Error('boom')) const Component = loadable(load) const { container } = render( @@ -211,14 +228,30 @@ describe('#loadable', () => { , ) expect(container).toBeEmpty() - load.reject(new Error('boom')) await wait(() => expect(container).toHaveTextContent('error')) }) + + it('supports retry from Error Boundary', async () => { + const load = jest + .fn() + .mockRejectedValueOnce(new Error('Error Boundary')) + .mockResolvedValueOnce({ default: () => 'loaded' }) + + const Component = loadable(load) + const { container } = render( + + + , + ) + expect(container).toBeEmpty() + + await wait(() => expect(container).toHaveTextContent('loaded')) + }) }) describe('#lazy', () => { it('supports Suspense', async () => { - const load = createLoadFunction() + const load = jest.fn().mockResolvedValue({ default: () => 'loaded' }) const Component = lazy(load) const { container } = render( @@ -226,20 +259,111 @@ describe('#lazy', () => { , ) expect(container).toHaveTextContent('progress') - load.resolve({ default: () => 'loaded' }) + await wait(() => expect(container).not.toHaveTextContent('progress')) + expect(container).toHaveTextContent('loaded') + }) + + it('should only render both components when both resolve', async () => { + const load = jest + .fn() + .mockResolvedValueOnce({ default: ({ text }) => text }) + ::mockDelayedResolvedValueOnce({ default: ({ text }) => text }) + + const Component = lazy(load) + + const { container } = render( + + <> + + + + , + ) + expect(container).toHaveTextContent('progress') + await wait(() => expect(container).not.toHaveTextContent('progress')) + expect(container.textContent).toBe('AB') + }) + + it("should render multiple elements of the same async component under contextual Suspense'", async () => { + const load = jest.fn().mockResolvedValue({ default: ({ text }) => text }) + const Component = lazy(load) + const { container } = render( + <> + + + + + + + , + ) + expect(container).toHaveTextContent('progressA progressB') + + await wait(() => expect(container).not.toHaveTextContent('progress')) + expect(container).toHaveTextContent('AB') + }) + + it("shouldn't trigger nested Suspense for same lazy component", async () => { + const load = jest.fn().mockResolvedValue({ default: ({ text }) => text }) + const Component = lazy(load) + const { container } = render( + <> + + + + + + + , + ) + expect(container.textContent).toBe('progressA') + + await wait(() => expect(container).not.toHaveTextContent('progressA')) + expect(container).toHaveTextContent('AB') + }) + + it('should support Error Boundary', async () => { + const load = jest.fn().mockRejectedValue(new Error('Error Boundary')) + const Component = lazy(load) + const { container } = render( + + + + + , + ) + expect(container).toHaveTextContent('progress') + await wait(() => expect(container).toHaveTextContent('error')) + }) + + it('should support retry from Error Boundary', async () => { + const load = jest + .fn() + .mockRejectedValueOnce(new Error('Error Boundary')) + .mockResolvedValueOnce({ default: () => 'loaded' }) + + const Component = lazy(load) + const { container } = render( + + + + + , + ) + expect(container).toHaveTextContent('progress') + await wait(() => expect(container).toHaveTextContent('loaded')) }) }) describe('#loadable.lib', () => { it('loads library as render prop', async () => { - const load = createLoadFunction() + const library = { it: 'is', a: 'lib' } + const load = jest.fn().mockResolvedValue(library) const Lib = loadable.lib(load) const renderFn = jest.fn(() => 'loaded') const { container } = render({renderFn}) expect(container).toBeEmpty() - const library = { it: 'is', a: 'lib' } - load.resolve(library) await wait(() => expect(container).toHaveTextContent('loaded')) expect(renderFn).toHaveBeenCalledWith(library) }) @@ -247,7 +371,8 @@ describe('#loadable.lib', () => { describe('#lazy.lib', () => { it('supports Suspense', async () => { - const load = createLoadFunction() + const library = { it: 'is', a: 'lib' } + const load = jest.fn().mockResolvedValue(library) const Lib = lazy.lib(load) const renderFn = jest.fn(() => 'loaded') const { container } = render( @@ -256,9 +381,21 @@ describe('#lazy.lib', () => { , ) expect(container).toHaveTextContent('progress') - const library = { it: 'is', a: 'lib' } - load.resolve(library) await wait(() => expect(container).toHaveTextContent('loaded')) - expect(container).toHaveTextContent('loaded') + }) + + it('supports Error Boundary', async () => { + const load = jest.fn().mockRejectedValue(new Error('Error Boundary')) + const Lib = lazy.lib(load) + const renderFn = jest.fn(() => 'loaded') + const { container } = render( + + + {renderFn} + + , + ) + expect(container).toHaveTextContent('progress') + await wait(() => expect(container).toHaveTextContent('error')) }) })