Fixed lazy usage with Suspense and Error Boundary together #521

kamikazePT · 2020-02-04T21:14:59Z

Summary

When using lazy ( the Suspense version of loadable ), if any error occured, the Suspense would keep rendering It's fallback and retrying and never being catched by the parent ErrorBoundary.

For example I have a dynamic Icon that has specific components, and depending on the Icon type, it loads the correct component, displaying a default icon if no component is found, the way to do this without an Error Boundary would be to add the catch clause on the import( ) statement.

Test plan

With this PR, if the import( ) fails because the file doesn't exist, it will then render the Fallback provided to the ErrorBoundary

kamikazePT · 2020-02-04T23:22:08Z

In the meantime I've found another issue regarding Suspense usage.

If I have a full dynamic import aka something like
lazy(props => import(./${props.name}))

and "name" is changing runtime, we would be blocked on a loop on the throw loadAsync call

PS: you need to define "key" prop on the component using the props of the path in order to force a new fetch

gregberge

Your fix is not correct. The correct fix is to cache the promise.

kamikazePT · 2020-02-17T16:37:47Z

EDIT: give me a bit, I had an idea

@gregberge yeah... but caching the promise, how would you get the result in a sync way to change this piece of code inside the render method? to cache the promise instead of the result/error the render method needs to be re-thinked, no?

      if (options.suspense && !this.promise) {
          const cachedResponse = this.getCache()
          if (!cachedResponse) throw this.loadAsync()
          if (cachedResponse.error) throw cachedResponse.error

          return render({
            loading: false,
            fallback: null,
            result: cachedResponse.result,
            options,
            props: { ...props, ref: forwardedRef },
          })
        }

kamikazePT · 2020-02-17T16:50:18Z

@gregberge
pushed changes :)

tests are passing, promise cached and render method looks cleaner

theKashey · 2020-02-21T08:20:31Z

There is another PR, which fixes the same problem - #516

But it has no tests :)

kamikazePT · 2020-02-21T11:20:38Z

Actually I think that PR is somewhat a complement to this one, he doesnt fix the error not being propagated to the error boundary, he fix's the re-fetching on rehydration after ssr

Atleast that was what I understood from the change he made which was on loadSync that runs on ssr

Still I dont believe that the cache persists from ssr to csr o.O which makes it suspend the same

theKashey · 2020-02-21T22:18:31Z

packages/component/src/createLoadable.js

-            props: { ...props, ref: forwardedRef },
-          })
+          const cachedPromise = this.getCache()
+          if (!cachedPromise) throw this.loadAsync()


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.

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?

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.

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

theKashey · 2020-02-21T22:19:48Z

packages/component/src/createLoadable.js

+
+          const cachedPromise = this.getCache()
+
+          this.promise = cachedPromise || ctor.requireAsync(props)


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.

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

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.

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')))

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.

packages/component/src/loadable.test.js

gregberge · 2020-02-22T07:37:43Z

Hello @kamikazePT, thank you for trying, but I think the approach is not the good one. The best one is to create a container that encapsulate the promise.

An example:

export const PENDING = 0
export const RESOLVED = 1
export const REJECTED = 2

function createContainer(status, result) {
  const container = {
    status,
    result,
    write(result) {
      if (container.status !== RESOLVED) {
        throw new Error('Cannot write container if not resolved')
      }
      return { ...container, result, toJSON: () => result }
    },
    toJSON() {
      return result
    },
  }
  return container
}

export function createContainerFromPromise(promise) {
  const container = createContainer(PENDING)
  container.promise = promise.then(
    r => {
      container.status = RESOLVED
      container.result = r
      container.toJSON = () => r
    },
    e => {
      container.status = REJECTED
      container.error = e
    },
  )
  return container
}

export function createContainerFromValue(value) {
  return createContainer(RESOLVED, value)
}

kamikazePT · 2020-02-26T21:31:44Z

@gregberge
@theKashey

So with some hammer magic I managed to make retrying work for both lazy and loadable variants

It needs some code cleaning but atleast it's tested and working at the moment...

PS: okay can I 0.02kb to the max size?

theKashey · 2020-02-29T21:57:00Z

packages/component/src/util.js

+export function statusAware(promise) {
+  let status = STATUS_PENDING
+
+  const proxy = new Proxy(promise, {


No Proxies please. They are not supported widely enough yet.
And why this needs a recursive wrapping?

The recursion is that if you call then or catch, it enables you to access status on the newly created promises from those calls

gregberge · 2020-04-05T18:52:34Z

@kamikazePT I am sorry but i don't have time to dig into this right now.

…n using suspense

kamikazePT · 2020-04-14T17:17:56Z

using a fork with this PR in a production app, just noticed I added a bug when fixed the retrying functionality ( using private npm feed, some outdated releases are in npmjs but wouldn't recommend it )

also tests weren't exactly bulletproof and could lead to false assumptions, they were improved as well

stale · 2020-06-14T10:26:42Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

theKashey · 2020-06-14T10:44:03Z

Let's double-check what's missing.

kamikazePT · 2020-08-04T13:17:38Z

@theKashey
PR updated with latest commits on master

theKashey · 2020-08-05T06:10:52Z

👀

kamikazePT · 2020-08-07T15:16:46Z

@theKashey

revisited the PR, removed the ES6 Proxy

Can you check if anything is missing or that needs refactoring?
So that this PR can move forward :D

kamikazePT · 2020-08-29T18:42:11Z

@theKashey
@gregberge

Anything you need to discuss regarding the solution? or the refactor in the unit tests mocks of the load function?

theKashey · 2020-09-03T10:02:30Z

Hey @kamikazePT, sorry for being so slow. Just two moments left:

:: operator used in test (is it "babel-bind"? Is there any way not to use it?)
adding callbacks to pending promises

kamikazePT · 2020-09-03T12:46:07Z

@theKashey

the bind operator is only there for sugar syntax and since it's only being used in testing environment, it won't add overhead to the bundling / transpiling for the release files, I would keep it :)

can you explain a bit more what you mean with adding callbacks for pending promises?

EDIT: but yes there's a way to not use the binding operator, either with mockDelayedResolvedValueOnce.bind(), or changing the function interface from mockDelayedResolvedValueOnce(resolvedValue) to mockDelayedResolvedValueOnce(mock, resolvedValue)
keeping the :: operator, we can use it as an extension method from the mock function, which looks nicer imo

theKashey · 2020-09-03T22:26:07Z

can you explain a bit more what you mean with adding callbacks for pending promises?

Every time you call .loadAsync it does .then It is possible to call it multiple times, due to multiple reasons, and it will "stack" callbacks, while only one is required.

kamikazePT · 2020-09-04T21:14:20Z

@theKashey

Fixed the callback situation and removed unnecessary bits of code that weren't doing anything (tests are passing)

Please check it again :)
Hopefully everything is sorted!

theKashey · 2020-09-05T00:09:01Z

Very good @kamikazePT 👍

theKashey · 2020-09-05T00:09:34Z

PS: 🥳

…berge#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

kamikazePT added 4 commits February 4, 2020 20:59

Fixed lazy usage with Suspense and Error Boundary together

03eb6d9

Typo

d920619

v1.0.0-fork

c50669e

accidental push

f85fcaa

kamikazePT requested a review from gregberge February 4, 2020 21:39

kamikazePT added 2 commits February 4, 2020 21:45

Condition was reversed

790ad66

Fixed Suspense with full dynamic import after fulfilled promise

cc0f646

Added unit test

addb38f

kamikazePT requested a review from theKashey February 7, 2020 17:52

gregberge requested changes Feb 16, 2020

View reviewed changes

cached promise

6ab1221

theKashey mentioned this pull request Feb 21, 2020

Handle of Loading chunk failure #525

Closed

theKashey reviewed Feb 21, 2020

View reviewed changes

packages/component/src/loadable.test.js Show resolved Hide resolved

kamikazePT added 2 commits February 22, 2020 01:52

added tests for multiple elements of same async component

3e30e69

renamed unit test

11d34e8

kamikazePT added 3 commits February 22, 2020 15:36

added retryable error boundary

dacc790

reworked tests

1ca1815

Retrying working for lazy and loadable

f9ef9c7

kamikazePT added 2 commits February 26, 2020 21:32

linter should run on pre-commit... :/

cf9763d

upped max bundle size

ac57625

theKashey reviewed Feb 29, 2020

View reviewed changes

fix: Fixed suspense tests and fixed un-throwable pending promises whe…

df7a4d3

…n using suspense

kamikazePT added 4 commits April 14, 2020 18:23

refactor: removed unnecessary wait in test

c888a5b

test: fixed test regarding nested suspense

97158e2

test: fixed fucked up assertion

4d41215

fix: fixed weird corner case on error boundary not being reached

df79086

stale bot added the wontfix label Jun 14, 2020

stale bot removed the wontfix label Jun 14, 2020

chore: merge fix

8d874f9

feat: removed proxy

d0d4c39

theKashey self-requested a review August 9, 2020 00:11

kamikazePT added 2 commits September 4, 2020 21:53

fix: removed unnecessary code

a15d07a

fix: fixed unnecesssary stack of callbacks

72b28dd

theKashey merged commit 42fbdd0 into gregberge:master Sep 5, 2020

theKashey mentioned this pull request Sep 5, 2020

Release 5.14 #625

Merged

kamikazePT deleted the error-boundary-suspense branch September 6, 2020 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed lazy usage with Suspense and Error Boundary together #521

Fixed lazy usage with Suspense and Error Boundary together #521

kamikazePT commented Feb 4, 2020

kamikazePT commented Feb 4, 2020 •

edited

gregberge left a comment

kamikazePT commented Feb 17, 2020 •

edited

kamikazePT commented Feb 17, 2020

theKashey commented Feb 21, 2020 •

edited

kamikazePT commented Feb 21, 2020 •

edited

theKashey Feb 21, 2020

kamikazePT Feb 21, 2020

theKashey Feb 22, 2020

kamikazePT Feb 22, 2020

theKashey Feb 21, 2020

kamikazePT Feb 21, 2020 •

edited

theKashey Feb 22, 2020

kamikazePT Feb 22, 2020 •

edited

theKashey Feb 23, 2020

gregberge commented Feb 22, 2020

kamikazePT commented Feb 26, 2020 •

edited

theKashey Feb 29, 2020

kamikazePT Feb 29, 2020 •

edited

gregberge commented Apr 5, 2020

kamikazePT commented Apr 14, 2020 •

edited

stale bot commented Jun 14, 2020

theKashey commented Jun 14, 2020

kamikazePT commented Aug 4, 2020

theKashey commented Aug 5, 2020

kamikazePT commented Aug 7, 2020 •

edited

kamikazePT commented Aug 29, 2020

theKashey commented Sep 3, 2020

kamikazePT commented Sep 3, 2020 •

edited

theKashey commented Sep 3, 2020

kamikazePT commented Sep 4, 2020

theKashey commented Sep 5, 2020

theKashey commented Sep 5, 2020


		const cachedPromise = this.getCache()

		this.promise = cachedPromise \|\| ctor.requireAsync(props)

Fixed lazy usage with Suspense and Error Boundary together #521

Fixed lazy usage with Suspense and Error Boundary together #521

Conversation

kamikazePT commented Feb 4, 2020

Summary

Test plan

kamikazePT commented Feb 4, 2020 • edited

gregberge left a comment

Choose a reason for hiding this comment

kamikazePT commented Feb 17, 2020 • edited

kamikazePT commented Feb 17, 2020

theKashey commented Feb 21, 2020 • edited

kamikazePT commented Feb 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamikazePT Feb 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamikazePT Feb 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregberge commented Feb 22, 2020

kamikazePT commented Feb 26, 2020 • edited

Choose a reason for hiding this comment

kamikazePT Feb 29, 2020 • edited

Choose a reason for hiding this comment

gregberge commented Apr 5, 2020

kamikazePT commented Apr 14, 2020 • edited

stale bot commented Jun 14, 2020

theKashey commented Jun 14, 2020

kamikazePT commented Aug 4, 2020

theKashey commented Aug 5, 2020

kamikazePT commented Aug 7, 2020 • edited

kamikazePT commented Aug 29, 2020

theKashey commented Sep 3, 2020

kamikazePT commented Sep 3, 2020 • edited

theKashey commented Sep 3, 2020

kamikazePT commented Sep 4, 2020

theKashey commented Sep 5, 2020

theKashey commented Sep 5, 2020

kamikazePT commented Feb 4, 2020 •

edited

kamikazePT commented Feb 17, 2020 •

edited

theKashey commented Feb 21, 2020 •

edited

kamikazePT commented Feb 21, 2020 •

edited

kamikazePT Feb 21, 2020 •

edited

kamikazePT Feb 22, 2020 •

edited

kamikazePT commented Feb 26, 2020 •

edited

kamikazePT Feb 29, 2020 •

edited

kamikazePT commented Apr 14, 2020 •

edited

kamikazePT commented Aug 7, 2020 •

edited

kamikazePT commented Sep 3, 2020 •

edited