From 42fbdd0d552551b18ed0781383bb0073e1cd8640 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 --- babel.config.js | 5 + package.json | 1 + packages/component/src/createLoadable.js | 87 +++++---- packages/component/src/library.js | 4 +- packages/component/src/loadable.test.js | 231 ++++++++++++++++++----- yarn.lock | 20 ++ 6 files changed, 263 insertions(+), 85 deletions(-) diff --git a/babel.config.js b/babel.config.js index 44d79417..ea8ce4a2 100644 --- a/babel.config.js +++ b/babel.config.js @@ -11,4 +11,9 @@ module.exports = { ['@babel/preset-env', { loose: true, targets: getTargets() }], ], plugins: ['@babel/plugin-proposal-class-properties'], + env: { + test: { + plugins: ['@babel/plugin-proposal-function-bind'], + }, + }, } diff --git a/package.json b/package.json index 3ddced5d..2bb23bdd 100644 --- a/package.json +++ b/package.json @@ -28,6 +28,7 @@ "@babel/core": "^7.7.7", "@babel/node": "^7.7.7", "@babel/plugin-proposal-class-properties": "^7.7.4", + "@babel/plugin-proposal-function-bind": "^7.8.3", "@babel/plugin-transform-runtime": "^7.7.6", "@babel/preset-env": "^7.7.7", "@babel/preset-react": "^7.7.4", diff --git a/packages/component/src/createLoadable.js b/packages/component/src/createLoadable.js index dd22c647..d77c0bfa 100644 --- a/packages/component/src/createLoadable.js +++ b/packages/component/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/packages/component/src/library.js b/packages/component/src/library.js index b7e82dec..8d945dc6 100644 --- a/packages/component/src/library.js +++ b/packages/component/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/packages/component/src/loadable.test.js b/packages/component/src/loadable.test.js index e400f150..b3691062 100644 --- a/packages/component/src/loadable.test.js +++ b/packages/component/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')) }) }) diff --git a/yarn.lock b/yarn.lock index 3b378e27..d32ba8b7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -185,6 +185,11 @@ resolved "https://registry.yarnpkg.com/@babel/helper-plugin-utils/-/helper-plugin-utils-7.0.0.tgz#bbb3fbee98661c569034237cc03967ba99b4f250" integrity sha512-CYAOUCARwExnEixLdB6sDm2dIJ/YgEAKDM1MOeMeZu9Ld/bDgVo8aiWrXwcY7OBh+1Ea2uUcVRcxKk0GJvW7QA== +"@babel/helper-plugin-utils@^7.8.3": + version "7.8.3" + resolved "https://registry.yarnpkg.com/@babel/helper-plugin-utils/-/helper-plugin-utils-7.8.3.tgz#9ea293be19babc0f52ff8ca88b34c3611b208670" + integrity sha512-j+fq49Xds2smCUNYmEHF9kGNkhbet6yVIBp4e6oeQpH1RUs/Ir06xUKzDjDkGcaaokPiTNs2JBWHjaE4csUkZQ== + "@babel/helper-regex@^7.0.0", "@babel/helper-regex@^7.4.4": version "7.5.5" resolved "https://registry.yarnpkg.com/@babel/helper-regex/-/helper-regex-7.5.5.tgz#0aa6824f7100a2e0e89c1527c23936c152cab351" @@ -300,6 +305,14 @@ "@babel/helper-plugin-utils" "^7.0.0" "@babel/plugin-syntax-dynamic-import" "^7.7.4" +"@babel/plugin-proposal-function-bind@^7.8.3": + version "7.8.3" + resolved "https://registry.yarnpkg.com/@babel/plugin-proposal-function-bind/-/plugin-proposal-function-bind-7.8.3.tgz#e34a1e984771b84b6e5322745edeadca7e500ced" + integrity sha512-6q7VAHJQa9x4P6Lm6h6KHoJUEhx2r1buFKseHICe0ogb1LWxducO4tsQp3hd/7BVBo485YBsn6tJnpuwWm/9cA== + dependencies: + "@babel/helper-plugin-utils" "^7.8.3" + "@babel/plugin-syntax-function-bind" "^7.8.3" + "@babel/plugin-proposal-json-strings@^7.7.4": version "7.7.4" resolved "https://registry.yarnpkg.com/@babel/plugin-proposal-json-strings/-/plugin-proposal-json-strings-7.7.4.tgz#7700a6bfda771d8dc81973249eac416c6b4c697d" @@ -353,6 +366,13 @@ dependencies: "@babel/helper-plugin-utils" "^7.0.0" +"@babel/plugin-syntax-function-bind@^7.8.3": + version "7.8.3" + resolved "https://registry.yarnpkg.com/@babel/plugin-syntax-function-bind/-/plugin-syntax-function-bind-7.8.3.tgz#17d722cd8efc9bb9cf8bc59327f2b26295b352f7" + integrity sha512-gEYag4Q3CfqlQcJQQw/KSWdV2husGOnIsOsRlyzkoaNqj2V/V/CSdSJDCGSl67oJ1bdIYP6TjORWPH561dSJpA== + dependencies: + "@babel/helper-plugin-utils" "^7.8.3" + "@babel/plugin-syntax-json-strings@^7.7.4": version "7.7.4" resolved "https://registry.yarnpkg.com/@babel/plugin-syntax-json-strings/-/plugin-syntax-json-strings-7.7.4.tgz#86e63f7d2e22f9e27129ac4e83ea989a382e86cc"