Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed lazy usage with Suspense and Error Boundary together #521

Merged
merged 24 commits into from
Sep 5, 2020
Merged

Fixed lazy usage with Suspense and Error Boundary together #521

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
03eb6d9
Fixed lazy usage with Suspense and Error Boundary together
kamikazePT Feb 4, 2020
d920619
Typo
kamikazePT Feb 4, 2020
c50669e
v1.0.0-fork
kamikazePT Feb 4, 2020
f85fcaa
accidental push
kamikazePT Feb 4, 2020
790ad66
Condition was reversed
kamikazePT Feb 4, 2020
cc0f646
Fixed Suspense with full dynamic import after fulfilled promise
kamikazePT Feb 4, 2020
addb38f
Added unit test
kamikazePT Feb 6, 2020
6ab1221
cached promise
kamikazePT Feb 17, 2020
3e30e69
added tests for multiple elements of same async component
kamikazePT Feb 22, 2020
11d34e8
renamed unit test
kamikazePT Feb 22, 2020
dacc790
added retryable error boundary
kamikazePT Feb 22, 2020
1ca1815
reworked tests
kamikazePT Feb 26, 2020
f9ef9c7
Retrying working for lazy and loadable
kamikazePT Feb 26, 2020
cf9763d
linter should run on pre-commit... :/
kamikazePT Feb 26, 2020
ac57625
upped max bundle size
kamikazePT Feb 28, 2020
df7a4d3
fix: Fixed suspense tests and fixed un-throwable pending promises whe…
kamikazePT Apr 14, 2020
c888a5b
refactor: removed unnecessary wait in test
kamikazePT Apr 14, 2020
97158e2
test: fixed test regarding nested suspense
kamikazePT Apr 14, 2020
4d41215
test: fixed fucked up assertion
kamikazePT Apr 14, 2020
df79086
fix: fixed weird corner case on error boundary not being reached
kamikazePT Apr 15, 2020
8d874f9
chore: merge fix
kamikazePT Aug 4, 2020
d0d4c39
feat: removed proxy
kamikazePT Aug 7, 2020
a15d07a
fix: removed unnecessary code
kamikazePT Sep 4, 2020
72b28dd
fix: fixed unnecesssary stack of callbacks
kamikazePT Sep 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions packages/component/src/createLoadable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,16 +151,19 @@ function createLoadable({ resolve = identity, render, onLoad }) {
loadAsync() {
if (!this.promise) {
const { __chunkExtractor, forwardedRef, ...props } = this.props
this.promise = ctor
.requireAsync(props)

const cachedPromise = this.getCache()

this.promise = cachedPromise || ctor.requireAsync(props)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the cache is a bit obsolete?
What if it was rejected by any reason?

Now you are not able to restart the operation.

Copy link
Contributor Author

@kamikazePT kamikazePT Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make the promise retryable if I wanted some async component to retry.. instead of re-rendering from the error boundary :) although there might be use cases for the re-render, I'll think for a bit about this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promise cannot be retryable once it resolved or rejected - those are final states.
And in case of some error the normal expectation nowadays is to throw that error, and handle it at the nearest boundary.

Copy link
Contributor Author

@kamikazePT kamikazePT Feb 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was what I had in mind when I said making the "promise" retryable

retry = (fn, retries = 3) =>
    fn().catch(err => 
            retries > 1 
                ? retry(fn, retries - 1) 
                : Promise.reject(err))

loadable(() => retry(() => import('foo')))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to have "retry" working out of the box, but usually, it's better to expose this ability to the user, as long as they might want to communicate the problem to the end-user.


if (options.suspense) this.setCache(this.promise)

this.promise = this.promise
.then(loadedModule => {
const result = resolve(loadedModule, { Loadable })
if (options.suspense) {
this.setCache(result)
}
this.safeSetState(
{
result: resolve(loadedModule, { Loadable }),
result,
loading: false,
},
() => this.triggerOnLoad(),
Expand All @@ -184,15 +187,8 @@ function createLoadable({ resolve = identity, render, onLoad }) {
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) throw this.loadAsync()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would happen if you have two components? What if this component would be rendered twice?
In both cases the second one would be "cached" and probably would not work.

Probably the cache should hold information about the status of a cached item.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have 2 components at the same tree level, they should have different Keys (like in arrays), having different Keys makes them different instances with their own internal cache

What are your thoughts on that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might code split absolutely everything, including a single Button, you can use 20 times on a single page.
So - it could be the same cache key, repeated 20 times.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some tests for multiple rendered components
Please check them out to see if I understood you correctly

}

if (error) {
Expand Down
54 changes: 53 additions & 1 deletion packages/component/src/loadable.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -29,6 +30,27 @@ class Catch extends React.Component {
}
}

class ErrorBoundary extends React.Component {
state = {
error: false,
}

componentDidCatch() {
this.setState({ error: true })
}

render() {
const { children, fallback } = this.props
const { error } = this.state

if (error) {
return fallback || null
}

return children || null
}
}

describe('#loadable', () => {
beforeEach(() => {
jest.spyOn(console, 'error').mockImplementation(() => {})
Expand Down Expand Up @@ -212,6 +234,21 @@ describe('#lazy', () => {
load.resolve({ default: () => 'loaded' })
await wait(() => expect(container).toHaveTextContent('loaded'))
})

it('supports Error Boundary', async () => {
const load = createLoadFunction()
const Component = lazy(load)
const { container } = render(
<ErrorBoundary fallback="error">
<React.Suspense fallback="progress">
<Component />
</React.Suspense>
</ErrorBoundary>,
)
expect(container).toHaveTextContent('progress')
load.reject(new Error('Error Boundary'))
await wait(() => expect(container).toHaveTextContent('error'))
})
})

describe('#loadable.lib', () => {
Expand Down Expand Up @@ -242,6 +279,21 @@ describe('#lazy.lib', () => {
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 = createLoadFunction()
const Lib = lazy.lib(load)
const renderFn = jest.fn(() => 'loaded')
const { container } = render(
<ErrorBoundary fallback="error">
<React.Suspense fallback="progress">
<Lib>{renderFn}</Lib>
</React.Suspense>
</ErrorBoundary>,
)
expect(container).toHaveTextContent('progress')
load.reject(new Error('Error Boundary'))
await wait(() => expect(container).toHaveTextContent('error'))
theKashey marked this conversation as resolved.
Show resolved Hide resolved
})
})