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.

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 initially suspended component under sCU #1660

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sventschui
Copy link
Member

@sventschui sventschui commented May 30, 2019

So the parking approach to suspense has an issue when the suspending component suspends on the first render (as lazy() does) and is under a component with an sCU that returns false. This is represented by the newly added should work throughout sCU when initially being suspended test.

My first approach was to patch the _parentDom attribute and then call forceUpdate() on the component that suspended. The problem with this is, that when this component is directly under Suspense that it will show up (modify the DOM) too soon.

The second approach (this PR) is to not patch the _parentDom but call forceUpdate() on the _ancestorComponent of the suspending component. This seems to work but feels wrong. It also breaks other tests since it will result in some render methods being called too often.

Other thoughts I will explore:

  • Patch _parentDom and call forceUpdate on all suspending components once all of them finished. This seems wrong as a suspending component might suspend again.

@ForsakenHarmony
Copy link
Member

guess it's similar to what we do for context

https://github.com/preactjs/preact/blob/master/src/create-context.js#L25

@sventschui
Copy link
Member Author

Yes it's similar but with the issue that we want to "postpone" the update of the dom but not the re-render because:

  • the update of the dom will result in the fallback being overriden if there is no DOM between <Suspense> and the suspending component
  • calling render as soon as possible is crucial as it might suspend multiple times

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants