-
-
Notifications
You must be signed in to change notification settings - Fork 886
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
Fix infinite loop in Suspense error handling on the server #1649
Conversation
|
Pull Request Test Coverage Report for Build 4520261937Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
@@ -552,7 +552,6 @@ export function SuspenseList(props: { | |||
|
|||
export function Suspense(props: { fallback?: string; children: string }) { | |||
let done: undefined | ((html?: string, error?: any) => boolean); | |||
let clean: any; |
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.
clean
wasn't being used anywhere anymore. maybe that's a mistake? not sure what it did
@@ -569,8 +568,8 @@ export function Suspense(props: { fallback?: string; children: string }) { | |||
}); | |||
function runSuspense() { | |||
setHydrateContext({ ...ctx, count: 0 }); | |||
o && cleanNode(o); |
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.
o
is always present. before 1.6.13 clean
was checked here
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.
Are we sure we can just remove it? I hoisted it up for sure and that caused the issue, but it was always there. I guess I can retest the original React Query scenario.
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.
pre 1.6.13
the current Owner
would get assigned as o
so it could be null
, now we are always creating a new owner so the condition is pointless.
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.
Also, github is showing the preview a bit weirdly. I didn't remove cleanNode(o)
, just o &&
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.
Oh I get it. Yeah I was looking at the wrong part. I guess the trickiest part is I change this mechanism completely in 1.7. It's going to be a bit interesting to figure out how to merge this back into next. It looks like I need to do another 1.6 release given the stuff I've broken though so I will go with this for now.
Summary
Fixes #1648
Suspense was attaching it's
onError
not to theo
new owner it created, but to the currentOwner
, which ends up being the parent ofo
. So when it tries to pass the error to the parent withrunWithOwner
, it's actually passing it to itself creating an infinite loop.How did you test this change?
I see there are no server tests for solid-js at the moment. But patching solid-js locally has solved this issue for me.
I'm not sure if I should maybe try adding one using
// @jest-environment node
comment to force node env with jest?Maybe after #1609 gets merged this could be improved.