From 419351045185320a9932a251c688e72af66b20eb Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Fri, 13 Dec 2019 20:03:09 +1100 Subject: [PATCH 1/3] refactor: add `flushPromises` to component tests --- packages/component/src/loadable.test.js | 52 +++++++++++++++++-------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/packages/component/src/loadable.test.js b/packages/component/src/loadable.test.js index a48083e84..dabb14970 100644 --- a/packages/component/src/loadable.test.js +++ b/packages/component/src/loadable.test.js @@ -17,6 +17,10 @@ function createLoadFunction() { return fn } +function flushPromises() { + return new Promise((resolve) => setImmediate(resolve)) +} + class Catch extends React.Component { state = { error: false } @@ -73,7 +77,8 @@ describe('#loadable', () => { const { container } = render() expect(container).toBeEmpty() load.resolve({ default: () => 'loaded' }) - await wait(() => expect(container).toHaveTextContent('loaded')) + await flushPromises() + expect(container).toHaveTextContent('loaded') }) it('supports preload', async () => { @@ -86,7 +91,8 @@ describe('#loadable', () => { const { container } = render() expect(container).toBeEmpty() load.resolve({ default: () => 'loaded' }) - await wait(() => expect(container).toHaveTextContent('loaded')) + await flushPromises() + expect(container).toHaveTextContent('loaded') expect(load).toHaveBeenCalledTimes(2) }) @@ -95,7 +101,8 @@ describe('#loadable', () => { const Component = loadable(load) const { container } = render() load.resolve(() => 'loaded') - await wait(() => expect(container).toHaveTextContent('loaded')) + await flushPromises() + expect(container).toHaveTextContent('loaded') }) it('forwards props', async () => { @@ -103,7 +110,8 @@ describe('#loadable', () => { const Component = loadable(load) const { container } = render() load.resolve({ default: ({ name }) => name }) - await wait(() => expect(container).toHaveTextContent('James Bond')) + await flushPromises() + expect(container).toHaveTextContent('James Bond') }) it('should update component if props change', async () => { @@ -111,9 +119,11 @@ describe('#loadable', () => { const Component = loadable(load) const { container } = render() load.resolve({ default: ({ value }) => value }) - await wait(() => expect(container).toHaveTextContent('first')) + await flushPromises() + expect(container).toHaveTextContent('first') render(, { container }) - await wait(() => expect(container).toHaveTextContent('second')) + await flushPromises() + expect(container).toHaveTextContent('second') expect(load).toHaveBeenCalledTimes(1) }) @@ -122,10 +132,12 @@ describe('#loadable', () => { const Component = loadable(load, { cacheKey: ({ value }) => value }) const { container } = render() load.resolve({ default: ({ value }) => value }) - await wait(() => expect(container).toHaveTextContent('first')) + await flushPromises() + expect(container).toHaveTextContent('first') expect(load).toHaveBeenCalledTimes(1) render(, { container }) - await wait(() => expect(container).toHaveTextContent('second')) + await flushPromises() + expect(container).toHaveTextContent('second') expect(load).toHaveBeenCalledTimes(2) }) @@ -137,10 +149,12 @@ describe('#loadable', () => { }) const { container } = render() load.resolve({ default: ({ value }) => value }) - await wait(() => expect(container).toHaveTextContent('first')) + await flushPromises() + expect(container).toHaveTextContent('first') expect(load).toHaveBeenCalledTimes(1) render(, { container }) - await wait(() => expect(container).toHaveTextContent('second')) + await flushPromises() + expect(container).toHaveTextContent('second') expect(load).toHaveBeenCalledTimes(2) }) @@ -154,17 +168,20 @@ describe('#loadable', () => { }) const Component = loadable(load, { cacheKey: ({ name }) => name }) const { container } = render() - await wait(() => expect(container).toHaveTextContent('A-0')) + await flushPromises() + expect(container).toHaveTextContent('A-0') expect(load).toHaveBeenCalledTimes(1) expect(load).toHaveBeenCalledWith({ name: 'A', id: 0 }) expect(A).toHaveBeenCalledTimes(1) expect(A).toHaveBeenCalledWith({ name: 'A', id: 0 }, {}) render(, { container }) - await wait(() => expect(container).toHaveTextContent('A-1')) + await flushPromises() + expect(container).toHaveTextContent('A-1') expect(A).toHaveBeenCalledTimes(2) expect(A).toHaveBeenCalledWith({ name: 'A', id: 1 }, {}) render(, { container }) - await wait(() => expect(container).toHaveTextContent('B-2')) + await flushPromises() + expect(container).toHaveTextContent('B-2') expect(load).toHaveBeenCalledTimes(2) expect(load).toHaveBeenCalledWith({ name: 'B', id: 2 }) expect(B).toHaveBeenCalledTimes(1) @@ -182,7 +199,8 @@ describe('#loadable', () => { load.resolve({ default: React.forwardRef((props, fref) =>
), }) - await wait(() => expect(ref.current.tagName).toBe('DIV')) + await flushPromises() + expect(ref.current.tagName).toBe('DIV') }) it('throws when an error occurs', async () => { @@ -195,7 +213,8 @@ describe('#loadable', () => { ) expect(container).toBeEmpty() load.reject(new Error('boom')) - await wait(() => expect(container).toHaveTextContent('error')) + await flushPromises() + expect(container).toHaveTextContent('error') }) }) @@ -223,7 +242,8 @@ describe('#loadable.lib', () => { expect(container).toBeEmpty() const library = { it: 'is', a: 'lib' } load.resolve(library) - await wait(() => expect(container).toHaveTextContent('loaded')) + await flushPromises() + expect(container).toHaveTextContent('loaded') expect(renderFn).toHaveBeenCalledWith(library) }) }) From 0226338073fbb4b3d69e8337c882de4dc643b302 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Fri, 13 Dec 2019 20:09:36 +1100 Subject: [PATCH 2/3] feat: `guard` option to cancel or delay loading Watchdog is useful to delay or cancel the process of loading the component. The loadable component does not switch to the loaded state until the watchdog resolves, and if the watchdog rejects, the loadable component enters the error state. --- packages/component/src/createLoadable.js | 14 ++++-- packages/component/src/loadable.test.js | 46 +++++++++++++++++++ .../src/pages/docs/api-loadable-component.mdx | 43 +++++++++++------ website/src/pages/docs/delay.mdx | 17 +++++-- website/src/pages/docs/timeout.mdx | 17 +++++-- 5 files changed, 114 insertions(+), 23 deletions(-) diff --git a/packages/component/src/createLoadable.js b/packages/component/src/createLoadable.js index 261a09acf..103a32495 100644 --- a/packages/component/src/createLoadable.js +++ b/packages/component/src/createLoadable.js @@ -33,6 +33,13 @@ function createLoadable({ resolve = identity, render, onLoad }) { return null } + function guard(importPromise, props) { + if (options.guard) { + return options.guard(importPromise.then(() => {}), props) + } + return importPromise.then(() => {}); + } + class InnerLoadable extends React.Component { static getDerivedStateFromProps(props, state) { const cacheKey = getCacheKey(props) @@ -151,9 +158,10 @@ function createLoadable({ resolve = identity, render, onLoad }) { loadAsync() { if (!this.promise) { const { __chunkExtractor, forwardedRef, ...props } = this.props - this.promise = ctor - .requireAsync(props) - .then(loadedModule => { + const importPromise = ctor.requireAsync(props); + this.promise = Promise.all([importPromise, guard(importPromise, this.props)]) + .then((results) => { + const loadedModule = results[0]; const result = resolve(loadedModule, { Loadable }) if (options.suspense) { this.setCache(result) diff --git a/packages/component/src/loadable.test.js b/packages/component/src/loadable.test.js index dabb14970..a1115d6e4 100644 --- a/packages/component/src/loadable.test.js +++ b/packages/component/src/loadable.test.js @@ -71,6 +71,52 @@ describe('#loadable', () => { expect(container).toHaveTextContent('prop fallback') }) + it('allows delaying load with guard', async () => { + const load = createLoadFunction() + const guard = createLoadFunction() + const Component = loadable(load, { + fallback: 'fallback', + guard, + }) + const { container } = render() + expect(container).toHaveTextContent('fallback') + load.resolve({ default: () => 'loaded' }) + expect(guard).toHaveBeenCalledWith( + new Promise(() => {}), + { someProp: '123', __chunkExtractor: undefined, forwardedRef: null } + ) + await flushPromises() + expect(container).toHaveTextContent('fallback') + guard.resolve() + await flushPromises() + expect(container).toHaveTextContent('loaded') + }) + + it('allows cancelling load with guard', async () => { + const load = createLoadFunction() + const guard = createLoadFunction() + const Component = loadable(load, { + fallback: 'fallback', + guard, + }) + const { container } = render(( + + + + )) + expect(container).toHaveTextContent('fallback') + load.resolve({ default: () => 'loaded' }) + expect(guard).toHaveBeenCalledWith( + new Promise(() => {}), + { someProp: '123', __chunkExtractor: undefined, forwardedRef: null } + ) + await flushPromises() + expect(container).toHaveTextContent('fallback') + guard.reject(new Error('timeout')) + await flushPromises() + expect(container).toHaveTextContent('error') + }) + it('mounts component when loaded', async () => { const load = createLoadFunction() const Component = loadable(load) diff --git a/website/src/pages/docs/api-loadable-component.mdx b/website/src/pages/docs/api-loadable-component.mdx index 4412bbc25..37844d0eb 100644 --- a/website/src/pages/docs/api-loadable-component.mdx +++ b/website/src/pages/docs/api-loadable-component.mdx @@ -10,13 +10,14 @@ order: 10 Create a loadable component. -| Arguments | Description | -| ------------------ | -------------------------------------------------------------------- | -| `loadFn` | The function call to load the component. | -| `options` | Optional options. | -| `options.fallback` | Fallback displayed during the loading. | -| `options.ssr` | If `false`, it will not be processed server-side. Default to `true`. | -| `options.cacheKey` | Cache key function (see [dynamic import](/docs/dynamic-import/)) | +| Arguments | Description | +| -------------------------- | -------------------------------------------------------------------- | +| `loadFn` | The function call to load the component. | +| `options` | Optional options. | +| `options.guard` | Function returning a promise that can delay or cancel loading. | +| `options.fallback` | Fallback displayed during the loading. | +| `options.ssr` | If `false`, it will not be processed server-side. Default to `true`. | +| `options.cacheKey` | Cache key function (see [dynamic import](/docs/dynamic-import/)) | ```js import loadable from '@loadable/component' @@ -24,6 +25,19 @@ import loadable from '@loadable/component' const OtherComponent = loadable(() => import('./OtherComponent')) ``` + +### `options.guard` + +`options.guard` is a function that creates a promise, which can delay or cancel the loading of the component. +It can be useful to implement timeouts or delays. + +It receives a promise as an argument that resolves or rejects when the asynchronous import succeeds or fails. +It also receives the props as the second argument. +When/if the returned promise resolves, the component is rendered normally. +When/if the returned promise rejects, the import is considered to have failed. + +For examples, see the documentation on implementing [delays](/docs/delay/) and [timeouts](/docs/timeout/). + ## lazy Create a loadable component "Suspense" ready. @@ -89,13 +103,14 @@ OtherComponent.load().then(() => { Create a loadable library. -| Arguments | Description | -| ------------------ | -------------------------------------------------------------------- | -| `loadFn` | The function call to load the component. | -| `options` | Optional options. | -| `options.fallback` | Fallback displayed during the loading. | -| `options.ssr` | If `false`, it will not be processed server-side. Default to `true`. | -| `options.cacheKey` | Cache key function (see [dynamic import](/docs/dynamic-import)) | +| Arguments | Description | +| -------------------------- | -------------------------------------------------------------------- | +| `loadFn` | The function call to load the component. | +| `options` | Optional options. | +| `options.guard` | Function returning a promise that can delay or cancel loading. | +| `options.fallback` | Fallback displayed during the loading. | +| `options.ssr` | If `false`, it will not be processed server-side. Default to `true`. | +| `options.cacheKey` | Cache key function (see [dynamic import](/docs/dynamic-import)) | ```js import loadable from '@loadable/component' diff --git a/website/src/pages/docs/delay.mdx b/website/src/pages/docs/delay.mdx index c122d73e5..84089a20f 100644 --- a/website/src/pages/docs/delay.mdx +++ b/website/src/pages/docs/delay.mdx @@ -6,14 +6,25 @@ order: 50 # Delay -To avoid flashing a loader if the loading is very fast, you could implement a minimum delay. There is no built-in API in `@loadable/component` but you could do it using [`p-min-delay`](https://github.com/sindresorhus/p-min-delay). +To avoid flashing a loader if the loading is very fast, you could implement a minimum delay. There is no built-in API in `@loadable/component` but you could do it using the [`guard` option](/docs/api-loadable-component/#optionsguard) and [`p-min-delay`](https://github.com/sindresorhus/p-min-delay). ```js import loadable from '@loadable/component' import pMinDelay from 'p-min-delay' // Wait a minimum of 200ms before loading home. -export const OtherComponent = loadable(() => - pMinDelay(import('./OtherComponent'), 200) +export const OtherComponent = loadable( + () => import('./OtherComponent'), + { + guard: (importPromise) => pMinDelay(importPromise, 200) + } +) + +// Wait a different minimum time before loading home, depending on props. +export const OtherComponent = loadable( + () => import('./OtherComponent'), + { + guard: (importPromise, props) => pMinDelay(importPromise, props.delay) + } ) ``` diff --git a/website/src/pages/docs/timeout.mdx b/website/src/pages/docs/timeout.mdx index d2388ea4e..2adc85e11 100644 --- a/website/src/pages/docs/timeout.mdx +++ b/website/src/pages/docs/timeout.mdx @@ -6,14 +6,25 @@ order: 55 # Timeout -Infinite loading is not good for user experience, to avoid it implementing a timeout is a good workaround. You can do it using a third party module like [`promise-timeout`](https://github.com/building5/promise-timeout): +Infinite loading is not good for user experience, to avoid it implementing a timeout is a good workaround. You can do it using the [`guard` option](/docs/api-loadable-component/#optionsguard) and a third party module like [`promise-timeout`](https://github.com/building5/promise-timeout): ```js import loadable from '@loadable/component' import { timeout } from 'promise-timeout' // Wait a maximum of 2s before sending an error. -export const OtherComponent = loadable(() => - timeout(import('./OtherComponent'), 2000) +export const OtherComponent = loadable( + () => import('./OtherComponent'), + { + guard: (importPromise) => timeout(importPromise, 2000) + } +) + +// Wait a different maximum time before sending an error, depending on props. +export const OtherComponent = loadable( + () => import('./OtherComponent'), + { + guard: (importPromise, props) => timeout(importPromise, props.timeout) + } ) ``` From 85b5f45e9662ac0d3eefb27a35e37e535f878449 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Fri, 13 Dec 2019 18:23:04 +1100 Subject: [PATCH 3/3] fix: throw error when wrapping an `import()` call Wrapping import calls in functions passed at arguments to `loadable` or `loadable.lib` was problematic because if the function did not directly return the result of the `import()` call, any transformations done on the return value did not work during SSR. BREAKING CHANGE: wrapping an `import()` call in a function passed to `loadable` or `loadable.lib` is no longer allowed --- .../src/__snapshots__/index.test.js.snap | 132 +++++++++++++----- packages/babel-plugin/src/index.js | 57 +++++++- packages/babel-plugin/src/index.test.js | 36 ++++- .../src/pages/docs/api-loadable-component.mdx | 7 + website/src/pages/docs/babel-plugin.mdx | 20 +++ 5 files changed, 207 insertions(+), 45 deletions(-) diff --git a/packages/babel-plugin/src/__snapshots__/index.test.js.snap b/packages/babel-plugin/src/__snapshots__/index.test.js.snap index 21b5aa42d..31c6dba59 100644 --- a/packages/babel-plugin/src/__snapshots__/index.test.js.snap +++ b/packages/babel-plugin/src/__snapshots__/index.test.js.snap @@ -580,12 +580,12 @@ exports[`plugin loadable.lib should be transpiled too 1`] = ` });" `; -exports[`plugin simple import in a complex promise should work 1`] = ` +exports[`plugin simple import should transform path into "chunk-friendly" name 1`] = ` "loadable({ resolved: {}, chunkName() { - return \\"ModA\\"; + return \\"foo-bar\\"; }, isReady(props) { @@ -602,9 +602,9 @@ exports[`plugin simple import in a complex promise should work 1`] = ` return false; }, - importAsync: () => timeout(import( - /* webpackChunkName: \\"ModA\\" */ - './ModA'), 2000), + importAsync: () => import( + /* webpackChunkName: \\"foo-bar\\" */ + '../foo/bar'), requireAsync(props) { const key = this.resolve(props); @@ -627,21 +627,21 @@ exports[`plugin simple import in a complex promise should work 1`] = ` resolve() { if (require.resolveWeak) { - return require.resolveWeak(\\"./ModA\\"); + return require.resolveWeak(\\"../foo/bar\\"); } - return eval('require.resolve')(\\"./ModA\\"); + return eval('require.resolve')(\\"../foo/bar\\"); } });" `; -exports[`plugin simple import should transform path into "chunk-friendly" name 1`] = ` +exports[`plugin simple import should work with * in name 1`] = ` "loadable({ resolved: {}, chunkName() { - return \\"foo-bar\\"; + return \`foo\`.replace(/[^a-zA-Z0-9_!§$()=\\\\-^°]+/g, \\"-\\"); }, isReady(props) { @@ -659,8 +659,8 @@ exports[`plugin simple import should transform path into "chunk-friendly" name 1 }, importAsync: () => import( - /* webpackChunkName: \\"foo-bar\\" */ - '../foo/bar'), + /* webpackChunkName: \\"foo\\" */ + \`./foo*\`), requireAsync(props) { const key = this.resolve(props); @@ -683,21 +683,21 @@ exports[`plugin simple import should transform path into "chunk-friendly" name 1 resolve() { if (require.resolveWeak) { - return require.resolveWeak(\\"../foo/bar\\"); + return require.resolveWeak(\`./foo*\`); } - return eval('require.resolve')(\\"../foo/bar\\"); + return eval('require.resolve')(\`./foo*\`); } });" `; -exports[`plugin simple import should work with * in name 1`] = ` +exports[`plugin simple import should work with + concatenation 1`] = ` "loadable({ resolved: {}, chunkName() { - return \`foo\`.replace(/[^a-zA-Z0-9_!§$()=\\\\-^°]+/g, \\"-\\"); + return \\"\\"; }, isReady(props) { @@ -715,8 +715,8 @@ exports[`plugin simple import should work with * in name 1`] = ` }, importAsync: () => import( - /* webpackChunkName: \\"foo\\" */ - \`./foo*\`), + /* webpackChunkName: \\"\\" */ + './Mod' + 'A'), requireAsync(props) { const key = this.resolve(props); @@ -739,21 +739,21 @@ exports[`plugin simple import should work with * in name 1`] = ` resolve() { if (require.resolveWeak) { - return require.resolveWeak(\`./foo*\`); + return require.resolveWeak('./Mod' + 'A'); } - return eval('require.resolve')(\`./foo*\`); + return eval('require.resolve')('./Mod' + 'A'); } });" `; -exports[`plugin simple import should work with + concatenation 1`] = ` +exports[`plugin simple import should work with template literal 1`] = ` "loadable({ resolved: {}, chunkName() { - return \\"\\"; + return \`ModA\`.replace(/[^a-zA-Z0-9_!§$()=\\\\-^°]+/g, \\"-\\"); }, isReady(props) { @@ -771,8 +771,8 @@ exports[`plugin simple import should work with + concatenation 1`] = ` }, importAsync: () => import( - /* webpackChunkName: \\"\\" */ - './Mod' + 'A'), + /* webpackChunkName: \\"ModA\\" */ + \`./ModA\`), requireAsync(props) { const key = this.resolve(props); @@ -795,21 +795,21 @@ exports[`plugin simple import should work with + concatenation 1`] = ` resolve() { if (require.resolveWeak) { - return require.resolveWeak('./Mod' + 'A'); + return require.resolveWeak(\`./ModA\`); } - return eval('require.resolve')('./Mod' + 'A'); + return eval('require.resolve')(\`./ModA\`); } });" `; -exports[`plugin simple import should work with template literal 1`] = ` +exports[`plugin simple import with "webpackChunkName" comment should use it 1`] = ` "loadable({ resolved: {}, chunkName() { - return \`ModA\`.replace(/[^a-zA-Z0-9_!§$()=\\\\-^°]+/g, \\"-\\"); + return \\"ChunkA\\"; }, isReady(props) { @@ -827,8 +827,8 @@ exports[`plugin simple import should work with template literal 1`] = ` }, importAsync: () => import( - /* webpackChunkName: \\"ModA\\" */ - \`./ModA\`), + /* webpackChunkName: \\"ChunkA\\" */ + './ModA'), requireAsync(props) { const key = this.resolve(props); @@ -851,16 +851,16 @@ exports[`plugin simple import should work with template literal 1`] = ` resolve() { if (require.resolveWeak) { - return require.resolveWeak(\`./ModA\`); + return require.resolveWeak(\\"./ModA\\"); } - return eval('require.resolve')(\`./ModA\`); + return eval('require.resolve')(\\"./ModA\\"); } });" `; -exports[`plugin simple import with "webpackChunkName" comment should use it 1`] = ` +exports[`plugin simple import with "webpackChunkName" comment should use it even if comment is separated by "," 1`] = ` "loadable({ resolved: {}, @@ -883,7 +883,7 @@ exports[`plugin simple import with "webpackChunkName" comment should use it 1`] }, importAsync: () => import( - /* webpackChunkName: \\"ChunkA\\" */ + /* webpackPrefetch: true, webpackChunkName: \\"ChunkA\\" */ './ModA'), requireAsync(props) { @@ -916,12 +916,12 @@ exports[`plugin simple import with "webpackChunkName" comment should use it 1`] });" `; -exports[`plugin simple import with "webpackChunkName" comment should use it even if comment is separated by "," 1`] = ` +exports[`plugin simple import with arrow function with body 1`] = ` "loadable({ resolved: {}, chunkName() { - return \\"ChunkA\\"; + return \\"ModA\\"; }, isReady(props) { @@ -938,8 +938,66 @@ exports[`plugin simple import with "webpackChunkName" comment should use it even return false; }, - importAsync: () => import( - /* webpackPrefetch: true, webpackChunkName: \\"ChunkA\\" */ + importAsync: () => { + return import( + /* webpackChunkName: \\"ModA\\" */ + './ModA'); + }, + + requireAsync(props) { + const key = this.resolve(props); + this.resolved[key] = false; + return this.importAsync(props).then(resolved => { + this.resolved[key] = true; + return resolved; + }); + }, + + requireSync(props) { + const id = this.resolve(props); + + if (typeof __webpack_require__ !== 'undefined') { + return __webpack_require__(id); + } + + return eval('module.require')(id); + }, + + resolve() { + if (require.resolveWeak) { + return require.resolveWeak(\\"./ModA\\"); + } + + return eval('require.resolve')(\\"./ModA\\"); + } + +});" +`; + +exports[`plugin simple import with async arrow function 1`] = ` +"loadable({ + resolved: {}, + + chunkName() { + return \\"ModA\\"; + }, + + isReady(props) { + const key = this.resolve(props); + + if (this.resolved[key] === false) { + return false; + } + + if (typeof __webpack_modules__ !== 'undefined') { + return !!__webpack_modules__[key]; + } + + return false; + }, + + importAsync: async () => import( + /* webpackChunkName: \\"ModA\\" */ './ModA'), requireAsync(props) { diff --git a/packages/babel-plugin/src/index.js b/packages/babel-plugin/src/index.js index 04e960f83..cca88903f 100644 --- a/packages/babel-plugin/src/index.js +++ b/packages/babel-plugin/src/index.js @@ -32,6 +32,49 @@ const loadablePlugin = api => { return imports } + function isImportCall(path) { + if (path.type === 'CallExpression') { + const { callee } = path; + if (callee.type === 'Import') { + return true; + } + } + return false + } + + function isFunctionBodyWhichReturnsImportCall(path) { + if (path.type === 'BlockStatement') { + const { body: methodBody } = path; + if (methodBody.length === 1) { + const [statement] = methodBody; + if (statement.type === 'ReturnStatement') { + const { argument: returnExpression } = statement; + if (isImportCall(returnExpression)) { + return true; + } + } + } + } + return false; + } + + function isFunctionAndOnlyReturnsImport(importCreator) { + if (importCreator.type === 'ArrowFunctionExpression') { + const { body } = importCreator; + if (isImportCall(body) || isFunctionBodyWhichReturnsImportCall(body)) { + return true; + } + } + + if (['ObjectMethod', 'FunctionExpression'].indexOf(importCreator.type) !== -1) { + const { body } = importCreator; + if (isFunctionBodyWhichReturnsImportCall(body)) { + return true; + } + } + return false; + } + const propertyFactories = properties.map(init => init(api)) function isValidIdentifier(path) { @@ -72,10 +115,18 @@ const loadablePlugin = api => { } function transformImport(path) { - const callPaths = collectImportCallPaths(path) + const importCreator = path.node.type === 'CallExpression' + ? path.node.arguments[0] // loadable((...) => import(...)) or loadable.lib + : path.node; // /* #__LOADABLE__ */ () => import(...) - // Ignore loadable function that does not have any "import" call - if (callPaths.length === 0) return + if (!isFunctionAndOnlyReturnsImport(importCreator)) { + throw new Error( + 'The first argument to `loadable()` must be a function with a single statement that returns a call to `import()`' + + 'See https://loadable-components.com/docs/api-loadable-component/#loadfn for more information', + ); + } + + const callPaths = collectImportCallPaths(path) // Multiple imports call is not supported if (callPaths.length > 1) { diff --git a/packages/babel-plugin/src/index.test.js b/packages/babel-plugin/src/index.test.js index 1bab8750e..538b87c2f 100644 --- a/packages/babel-plugin/src/index.test.js +++ b/packages/babel-plugin/src/index.test.js @@ -73,13 +73,39 @@ describe('plugin', () => { }) }) - describe('in a complex promise', () => { - it('should work', () => { - const result = testPlugin(` + it('with arrow function with body', () => { + const result = testPlugin(` + loadable(() => { return import('./ModA') }) + `) + + expect(result).toMatchSnapshot() + }) + + it('with async arrow function', () => { + const result = testPlugin(` + loadable(async () => import('./ModA')) + `) + + expect(result).toMatchSnapshot() + }) + }) + + describe('reject wrapping of import', () => { + describe('in a wrapped promise', () => { + it('should fail', () => { + expect(() => testPlugin(` loadable(() => timeout(import('./ModA'), 2000)) - `) + `)).toThrow() + }) + }) - expect(result).toMatchSnapshot() + describe('returning an await statement', () => { + it('should fail', () => { + expect(() => testPlugin(` + loadable(async () => { + return await import('./ModA'); + }) + `)).toThrow() }) }) }) diff --git a/website/src/pages/docs/api-loadable-component.mdx b/website/src/pages/docs/api-loadable-component.mdx index 37844d0eb..9fbef582f 100644 --- a/website/src/pages/docs/api-loadable-component.mdx +++ b/website/src/pages/docs/api-loadable-component.mdx @@ -25,6 +25,13 @@ import loadable from '@loadable/component' const OtherComponent = loadable(() => import('./OtherComponent')) ``` +### `loadFn` +This is a function that returns a promise which resolves to the module where the component is exported. + +**Note:** It is highly recommended that you do not do anything in this function except returning an `import()` statement. +This is because such functionality [will not work](/docs/babel-plugin/#detection-of-wrapped-imports) when using the [Babel plugin](/docs/babel-plugin/) or during [server side rendering](/docs/server-side-rendering/). +For most use cases you can instead use the [`guard`](/docs/api-loadable-component/#optionsguard) option (e.g. for delays/timeouts), or the [`resolveComponent`](/docs/api-loadable-component/#optionsresolvecomponent) option (e.g. for wrapping the imported component). + ### `options.guard` diff --git a/website/src/pages/docs/babel-plugin.mdx b/website/src/pages/docs/babel-plugin.mdx index 939b15252..1ce1c4a1c 100644 --- a/website/src/pages/docs/babel-plugin.mdx +++ b/website/src/pages/docs/babel-plugin.mdx @@ -81,6 +81,26 @@ On client side, the two code are completely compatible. Please note that babel must not be configured [to strip comments](https://babeljs.io/docs/en/options#comments), since the chunk name is defined in a comment. +## Detection of wrapped imports + +During server side rendering, it is necessary for imported components to be resolved synchronously. +Normally, the Babel plugin automatically transforms asynchronous `import()` statements into the appropriate code to synchronously load the component on the server side. + +However, there are some patterns which are not compatible with this process. +To work consistently in server side rendering, the [`loadFn`](/docs/api-loadable-component/#loadable) must **directly** return an import statement. +For example, the following code will cause an error when using the babel plugin: + +```js +loadable(() => import('./component').then((imported) => imported)) +loadable(() => pMinDelay(import('./component'))) +``` + +To avoid this error the `loadFn` must directly return an import promise: + +```js +loadable(() => import('./component')) +``` + ## Loadable detection The detection of a loadable component is based on the keyword "loadable". It is an opinionated choice, it gives you flexibility but it could also be restrictive.