Skip to content

Fix initially suspended component under sCU#1660

Draft
sventschui wants to merge 1 commit into
mainfrom
bugfix/parking-suspense-parent-dom
Draft

Fix initially suspended component under sCU#1660
sventschui wants to merge 1 commit into
mainfrom
bugfix/parking-suspense-parent-dom

Conversation

@sventschui

@sventschui sventschui commented May 30, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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