-
-
Notifications
You must be signed in to change notification settings - Fork 380
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.
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
Fixed lazy usage with Suspense and Error Boundary together #521
Fixed lazy usage with Suspense and Error Boundary together #521
Conversation
In the meantime I've found another issue regarding Suspense usage. If I have a full dynamic import aka something like 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your fix is not correct. The correct fix is to cache the promise.
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?
|
@gregberge tests are passing, promise cached and render method looks cleaner |
There is another PR, which fixes the same problem - #516 But it has no tests :) |
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 |
props: { ...props, ref: forwardedRef }, | ||
}) | ||
const cachedPromise = this.getCache() | ||
if (!cachedPromise) throw this.loadAsync() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
||
const cachedPromise = this.getCache() | ||
|
||
this.promise = cachedPromise || ctor.requireAsync(props) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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')))
There was a problem hiding this comment.
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.
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)
} |
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? |
packages/component/src/util.js
Outdated
export function statusAware(promise) { | ||
let status = STATUS_PENDING | ||
|
||
const proxy = new Proxy(promise, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Proxies
please. They are not supported widely enough yet.
And why this needs a recursive wrapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recursion is that if you call then or catch, it enables you to access status on the newly created promises from those calls
@kamikazePT I am sorry but i don't have time to dig into this right now. |
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 |
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. |
Let's double-check what's missing. |
@theKashey |
👀 |
revisited the PR, removed the ES6 Proxy Can you check if anything is missing or that needs refactoring? |
Anything you need to discuss regarding the solution? or the refactor in the unit tests mocks of the load function? |
Hey @kamikazePT, sorry for being so slow. Just two moments left:
|
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) |
Every time you call |
Fixed the callback situation and removed unnecessary bits of code that weren't doing anything (tests are passing) Please check it again :) |
Very good @kamikazePT 👍 |
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
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