Hide timed-out children instead of deleting them so their state is preserved #13823
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
acdlite
force-pushed
the
placeholder-children
branch
from
1006724
to
a8c54b5
Compare
8 tasks
|
ReactDOM: size: 🔺+2.2%, gzip: 🔺+2.4% Details of bundled changes.Comparing: 21a79a1...f06c005 react-dom
react-art
react-test-renderer
react-noop-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
sebmarkbage
reviewed
Oct 11, 2018
| finishedWork.stateNode = {timedOutAt: currentTime}; | ||
| // which the children timed out. | ||
| // $FlowFixMe - Intentionally using a value other than an UpdateQueue. | ||
| finishedWork.updateQueue = {timedOutAt: requestCurrentTime()}; |
There was a problem hiding this comment.
If we use this field for something other than the UpdateQueue, we must never use this field in any generic branches. I.e. we can't have any code that checks .updateQueue for a bunch of different or all tag types.
What's your motivation though? This doesn't make sense to me. It is a signal that stores a stateful value over time and never is allowed to be reset when fully processed. Sounds like state to me. Either stateNode or memoizedState.
There was a problem hiding this comment.
Now that you mention it, memoizedState does seem to make the most sense. Good call.
acdlite
force-pushed
the
placeholder-children
branch
3 times, most recently
from
d9b589b
to
b64734d
Compare
acdlite
changed the title
|
Ok this is ready for review. I don't know how to implement the hide/unhide methods for React Native so I've left those empty for now. |
acdlite
force-pushed
the
placeholder-children
branch
2 times, most recently
from
7400fee
to
46386e2
Compare
|
So this approach uses |
When unhiding, it will either apply the user-provided react/packages/react-dom/src/client/ReactDOMHostConfig.js Lines 432 to 445 in fe89105 We considered detaching the DOM nodes but you lose more state with that approach. Like uncontrolled form inputs. |
|
@acdlite I'm curious to what state we lose. I thought form elements (inputs, selects etc) keep their state when in a detached state. I see us running into user-land styling issues when playing with |
|
@trueadm Oh maybe I'm wrong about that one. We lose iframe state, and I believe we lose both form inputs and video playback. I'll check again. |
|
@acdlite I expect iframes to lose state, there was an earlier proposal to allow them to not do so but I think it was scrapped. Video elements shouldn't lose state anymore, they should pause if I remember from memory (apart from on iOS, which used to cause them to reload even if you apply |
|
@trueadm Sounds good. I'll defer to you and @sebmarkbage's judgment. |
gaearon
reviewed
Oct 12, 2018
| @@ -371,6 +521,7 @@ function completeWork( | |||
| break; | |||
| } | |||
| case HostRoot: { | |||
| updateHostContainer(workInProgress); | |||
There was a problem hiding this comment.
Is host environment throws, does it mean we don't clean up the stack due to pops being moved after?
There was a problem hiding this comment.
I'll revert this, mistake when resolving conflicts
|
Do we know that all code paths in new traversal loops are covered by test? Last time we added traversal (context) it took a few releases to flush the bugs out. Maybe there's some fuzz that can help? |
acdlite
force-pushed
the
placeholder-children
branch
from
add3f57
to
68f9ccc
Compare
Originally I did this to free the `stateNode` field to store a second set of children. I don't we'll need this anymore, since we use fragment fibers instead. But I still think using `updateQueue` makes more sense so I'll leave this in.
If the children timeout, we switch to showing the fallback children in place of the "primary" children. However, we don't want to delete the primary children because then their state will be lost (both the React state and the host state, e.g. uncontrolled form inputs). Instead we keep them mounted and hide them. Both the fallback children AND the primary children are rendered at the same time. Once the primary children are un-suspended, we can delete the fallback children — don't need to preserve their state. The two sets of children are siblings in the host environment, but semantically, for purposes of reconciliation, they are two separate sets. So we store them using two fragment fibers. However, we want to avoid allocating extra fibers for every placeholder. They're only necessary when the children time out, because that's the only time when both sets are mounted. So, the extra fragment fibers are only used if the children time out. Otherwise, we render the primary children directly. This requires some custom reconciliation logic to preserve the state of the primary children. It's essentially a very basic form of re-parenting.
acdlite
force-pushed
the
placeholder-children
branch
2 times, most recently
from
9f93c3c
to
3079cd2
Compare
SuspenseComponent has three pieces of state: - alreadyCaptured: Whether a component in the child subtree already suspended. If true, subsequent suspends should bubble up to the next boundary. - didTimeout: Whether the boundary renders the primary or fallback children. This is separate from `alreadyCaptured` because outside of strict mode, when a boundary times out, the first commit renders the primary children in an incomplete state, then performs a second commit to switch the fallback. In that first commit, `alreadyCaptured` is false and `didTimeout` is true. - timedOutAt: The time at which the boundary timed out. This is separate from `didTimeout` because it's not set unless the boundary actually commits. These were previously spread across several fields. This happens to make the non-strict case a bit less hacky; the logic for that special case is now mostly localized to the UnwindWork module.
acdlite
force-pushed
the
placeholder-children
branch
from
3079cd2
to
0389330
Compare
acdlite
force-pushed
the
placeholder-children
branch
3 times, most recently
from
95217d7
to
36ce895
Compare
This will be used in subsequent commits to test that timed-out children are properly hidden. Also adds getChildrenAsJSX() method as an alternative to using getChildren(). (Ideally all our tests would use test renderer #oneday.)
When a subtree takes too long to load, we swap its contents out for a fallback to unblock the rest of the tree. Because we don't want to lose the state of the timed out view, we shouldn't actually delete the nodes from the tree. Instead, we'll keep them mounted and hide them visually. When the subtree is unblocked, we un-hide it, having preserved the existing state. Adds additional host config methods. For mutation mode: - hideInstance - hideTextInstance - unhideInstance - unhideTextInstance For persistent mode: - cloneHiddenInstance - cloneUnhiddenInstance - createHiddenTextInstance I've only implemented the new methods in the noop and test renderers. I'll implement them in the other renderers in subsequent commits.
For DOM nodes, we hide using `el.style.display = 'none'`. Text nodes don't have style, so we hide using `text.textContent = ''`.
Need to distinguish mount from update. An unfortunate edge case :(
In non-concurrent mode, indeterminate fibers may commit in an inconsistent state. But when they update, we should throw out the old fiber and start fresh. Which means the new fiber needs a placement effect.
acdlite
force-pushed
the
placeholder-children
branch
from
36ce895
to
62a3dbe
Compare
sebmarkbage
reviewed
Oct 18, 2018
| // effects on the children. This is super weird but reconcileChildren is | ||
| // deeply nested and it doesn't seem worth it to refactor for this edge | ||
| // case. (There are unit tests to prevent regressions.) | ||
| current = null; |
There was a problem hiding this comment.
Instead of assigning this, we should just pass null in all places below. So the compiler knows.
sebmarkbage
approved these changes
Oct 18, 2018
|
|
||
| await advanceTimers(1000); | ||
|
|
||
| expect(container.textContent).toEqual('ABC'); |
There was a problem hiding this comment.
@acdlite Should we add expect for 0 < time < 500 and 500 < time < 1000 just like Suspense-test does?
There was a problem hiding this comment.
And I'm a little confused here, if you could tell me I would be very happy!
Why textContent is Loading... after execute ReactDOM.render immediately? IMO, it should shows Loading... only after time >= 500 just like how Suspense works, doesn't it?
There was a problem hiding this comment.
This test is not running in concurrent mode, so placeholders are shown immediately (sync).
There was a problem hiding this comment.
@bvaughn Thanks, I don't notice that. But if so, seems we don't need to set the maxDuration prop?
aweary
reviewed
Oct 19, 2018
| ); | ||
| if (_current !== null) { | ||
| // An indeterminate component only mounts if it suspended inside a non- | ||
| // concurrent tree, in an inconsistent state. We want to tree it like |
3 tasks
linjiajian999
pushed a commit
to linjiajian999/react
that referenced
this pull request
Oct 22, 2018
…eserved (facebook#13823) * Store the start time on `updateQueue` instead of `stateNode` Originally I did this to free the `stateNode` field to store a second set of children. I don't we'll need this anymore, since we use fragment fibers instead. But I still think using `updateQueue` makes more sense so I'll leave this in. * Use fragment fibers to keep the primary and fallback children separate If the children timeout, we switch to showing the fallback children in place of the "primary" children. However, we don't want to delete the primary children because then their state will be lost (both the React state and the host state, e.g. uncontrolled form inputs). Instead we keep them mounted and hide them. Both the fallback children AND the primary children are rendered at the same time. Once the primary children are un-suspended, we can delete the fallback children — don't need to preserve their state. The two sets of children are siblings in the host environment, but semantically, for purposes of reconciliation, they are two separate sets. So we store them using two fragment fibers. However, we want to avoid allocating extra fibers for every placeholder. They're only necessary when the children time out, because that's the only time when both sets are mounted. So, the extra fragment fibers are only used if the children time out. Otherwise, we render the primary children directly. This requires some custom reconciliation logic to preserve the state of the primary children. It's essentially a very basic form of re-parenting. * Use `memoizedState` to store various pieces of SuspenseComponent's state SuspenseComponent has three pieces of state: - alreadyCaptured: Whether a component in the child subtree already suspended. If true, subsequent suspends should bubble up to the next boundary. - didTimeout: Whether the boundary renders the primary or fallback children. This is separate from `alreadyCaptured` because outside of strict mode, when a boundary times out, the first commit renders the primary children in an incomplete state, then performs a second commit to switch the fallback. In that first commit, `alreadyCaptured` is false and `didTimeout` is true. - timedOutAt: The time at which the boundary timed out. This is separate from `didTimeout` because it's not set unless the boundary actually commits. These were previously spread across several fields. This happens to make the non-strict case a bit less hacky; the logic for that special case is now mostly localized to the UnwindWork module. * Hide timed-out Suspense children When a subtree takes too long to load, we swap its contents out for a fallback to unblock the rest of the tree. Because we don't want to lose the state of the timed out view, we shouldn't actually delete the nodes from the tree. Instead, we'll keep them mounted and hide them visually. When the subtree is unblocked, we un-hide it, having preserved the existing state. Adds additional host config methods. For mutation mode: - hideInstance - hideTextInstance - unhideInstance - unhideTextInstance For persistent mode: - cloneHiddenInstance - cloneUnhiddenInstance - createHiddenTextInstance I've only implemented the new methods in the noop and test renderers. I'll implement them in the other renderers in subsequent commits. * Include `hidden` prop in noop renderer's output This will be used in subsequent commits to test that timed-out children are properly hidden. Also adds getChildrenAsJSX() method as an alternative to using getChildren(). (Ideally all our tests would use test renderer #oneday.) * Implement hide/unhide host config methods for DOM renderer For DOM nodes, we hide using `el.style.display = 'none'`. Text nodes don't have style, so we hide using `text.textContent = ''`. * Implement hide/unhide host config methods for Art renderer * Create DOM fixture that tests state preservation of timed out content * Account for class components that suspend outside concurrent mode Need to distinguish mount from update. An unfortunate edge case :( * Fork appendAllChildren between persistent and mutation mode * Remove redundant check for existence of el.style * Schedule placement effect on indeterminate components In non-concurrent mode, indeterminate fibers may commit in an inconsistent state. But when they update, we should throw out the old fiber and start fresh. Which means the new fiber needs a placement effect. * Pass null instead of current everywhere in mountIndeterminateComponent
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
acdlite
added a commit
that referenced
this pull request
Nov 8, 2018
jetoneza
pushed a commit
to jetoneza/react
that referenced
this pull request
Jan 23, 2019
…eserved (facebook#13823) * Store the start time on `updateQueue` instead of `stateNode` Originally I did this to free the `stateNode` field to store a second set of children. I don't we'll need this anymore, since we use fragment fibers instead. But I still think using `updateQueue` makes more sense so I'll leave this in. * Use fragment fibers to keep the primary and fallback children separate If the children timeout, we switch to showing the fallback children in place of the "primary" children. However, we don't want to delete the primary children because then their state will be lost (both the React state and the host state, e.g. uncontrolled form inputs). Instead we keep them mounted and hide them. Both the fallback children AND the primary children are rendered at the same time. Once the primary children are un-suspended, we can delete the fallback children — don't need to preserve their state. The two sets of children are siblings in the host environment, but semantically, for purposes of reconciliation, they are two separate sets. So we store them using two fragment fibers. However, we want to avoid allocating extra fibers for every placeholder. They're only necessary when the children time out, because that's the only time when both sets are mounted. So, the extra fragment fibers are only used if the children time out. Otherwise, we render the primary children directly. This requires some custom reconciliation logic to preserve the state of the primary children. It's essentially a very basic form of re-parenting. * Use `memoizedState` to store various pieces of SuspenseComponent's state SuspenseComponent has three pieces of state: - alreadyCaptured: Whether a component in the child subtree already suspended. If true, subsequent suspends should bubble up to the next boundary. - didTimeout: Whether the boundary renders the primary or fallback children. This is separate from `alreadyCaptured` because outside of strict mode, when a boundary times out, the first commit renders the primary children in an incomplete state, then performs a second commit to switch the fallback. In that first commit, `alreadyCaptured` is false and `didTimeout` is true. - timedOutAt: The time at which the boundary timed out. This is separate from `didTimeout` because it's not set unless the boundary actually commits. These were previously spread across several fields. This happens to make the non-strict case a bit less hacky; the logic for that special case is now mostly localized to the UnwindWork module. * Hide timed-out Suspense children When a subtree takes too long to load, we swap its contents out for a fallback to unblock the rest of the tree. Because we don't want to lose the state of the timed out view, we shouldn't actually delete the nodes from the tree. Instead, we'll keep them mounted and hide them visually. When the subtree is unblocked, we un-hide it, having preserved the existing state. Adds additional host config methods. For mutation mode: - hideInstance - hideTextInstance - unhideInstance - unhideTextInstance For persistent mode: - cloneHiddenInstance - cloneUnhiddenInstance - createHiddenTextInstance I've only implemented the new methods in the noop and test renderers. I'll implement them in the other renderers in subsequent commits. * Include `hidden` prop in noop renderer's output This will be used in subsequent commits to test that timed-out children are properly hidden. Also adds getChildrenAsJSX() method as an alternative to using getChildren(). (Ideally all our tests would use test renderer #oneday.) * Implement hide/unhide host config methods for DOM renderer For DOM nodes, we hide using `el.style.display = 'none'`. Text nodes don't have style, so we hide using `text.textContent = ''`. * Implement hide/unhide host config methods for Art renderer * Create DOM fixture that tests state preservation of timed out content * Account for class components that suspend outside concurrent mode Need to distinguish mount from update. An unfortunate edge case :( * Fork appendAllChildren between persistent and mutation mode * Remove redundant check for existence of el.style * Schedule placement effect on indeterminate components In non-concurrent mode, indeterminate fibers may commit in an inconsistent state. But when they update, we should throw out the old fiber and start fresh. Which means the new fiber needs a placement effect. * Pass null instead of current everywhere in mountIndeterminateComponent
jetoneza
pushed a commit
to jetoneza/react
that referenced
this pull request
Jan 23, 2019
…acebook#14157) Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
n8schloss
pushed a commit
to n8schloss/react
that referenced
this pull request
Jan 31, 2019
…acebook#14157) Vestigial behavior that should have been removed in facebook#13823. Found using the Suspense fuzz tester in facebook#14147.
NMinhNguyen
referenced
this pull request
in enzymejs/react-shallow-renderer
Jan 29, 2020
…eserved (#13823) * Store the start time on `updateQueue` instead of `stateNode` Originally I did this to free the `stateNode` field to store a second set of children. I don't we'll need this anymore, since we use fragment fibers instead. But I still think using `updateQueue` makes more sense so I'll leave this in. * Use fragment fibers to keep the primary and fallback children separate If the children timeout, we switch to showing the fallback children in place of the "primary" children. However, we don't want to delete the primary children because then their state will be lost (both the React state and the host state, e.g. uncontrolled form inputs). Instead we keep them mounted and hide them. Both the fallback children AND the primary children are rendered at the same time. Once the primary children are un-suspended, we can delete the fallback children — don't need to preserve their state. The two sets of children are siblings in the host environment, but semantically, for purposes of reconciliation, they are two separate sets. So we store them using two fragment fibers. However, we want to avoid allocating extra fibers for every placeholder. They're only necessary when the children time out, because that's the only time when both sets are mounted. So, the extra fragment fibers are only used if the children time out. Otherwise, we render the primary children directly. This requires some custom reconciliation logic to preserve the state of the primary children. It's essentially a very basic form of re-parenting. * Use `memoizedState` to store various pieces of SuspenseComponent's state SuspenseComponent has three pieces of state: - alreadyCaptured: Whether a component in the child subtree already suspended. If true, subsequent suspends should bubble up to the next boundary. - didTimeout: Whether the boundary renders the primary or fallback children. This is separate from `alreadyCaptured` because outside of strict mode, when a boundary times out, the first commit renders the primary children in an incomplete state, then performs a second commit to switch the fallback. In that first commit, `alreadyCaptured` is false and `didTimeout` is true. - timedOutAt: The time at which the boundary timed out. This is separate from `didTimeout` because it's not set unless the boundary actually commits. These were previously spread across several fields. This happens to make the non-strict case a bit less hacky; the logic for that special case is now mostly localized to the UnwindWork module. * Hide timed-out Suspense children When a subtree takes too long to load, we swap its contents out for a fallback to unblock the rest of the tree. Because we don't want to lose the state of the timed out view, we shouldn't actually delete the nodes from the tree. Instead, we'll keep them mounted and hide them visually. When the subtree is unblocked, we un-hide it, having preserved the existing state. Adds additional host config methods. For mutation mode: - hideInstance - hideTextInstance - unhideInstance - unhideTextInstance For persistent mode: - cloneHiddenInstance - cloneUnhiddenInstance - createHiddenTextInstance I've only implemented the new methods in the noop and test renderers. I'll implement them in the other renderers in subsequent commits. * Include `hidden` prop in noop renderer's output This will be used in subsequent commits to test that timed-out children are properly hidden. Also adds getChildrenAsJSX() method as an alternative to using getChildren(). (Ideally all our tests would use test renderer #oneday.) * Implement hide/unhide host config methods for DOM renderer For DOM nodes, we hide using `el.style.display = 'none'`. Text nodes don't have style, so we hide using `text.textContent = ''`. * Implement hide/unhide host config methods for Art renderer * Create DOM fixture that tests state preservation of timed out content * Account for class components that suspend outside concurrent mode Need to distinguish mount from update. An unfortunate edge case :( * Fork appendAllChildren between persistent and mutation mode * Remove redundant check for existence of el.style * Schedule placement effect on indeterminate components In non-concurrent mode, indeterminate fibers may commit in an inconsistent state. But when they update, we should throw out the old fiber and start fresh. Which means the new fiber needs a placement effect. * Pass null instead of current everywhere in mountIndeterminateComponent
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the children timeout, we switch to showing the fallback children in place of the "primary" children. However, we don't want to delete the primary children because then their state will be lost (both the React state and the host state, e.g. uncontrolled form inputs). Instead we keep them mounted and hide them. Both the fallback children AND the primary children are rendered at the same time. Once the primary children are un-suspended, we can delete the fallback children — don't need to preserve their state.
The two sets of children are siblings in the host environment, but semantically, for purposes of reconciliation, they are two separate sets. So we store them using two fragment fibers.
However, we want to avoid allocating two extra fibers for every Suspense fiber. They're only necessary when the children time out, because that's the only time when both sets are mounted.
So, the extra fragment fibers are only used if the children time out. Otherwise, we render the primary children directly. This requires some custom reconciliation logic to preserve the state of the primary children. It's essentially a very basic form of re-parenting.
memoizedState.