Reuse hooks when replaying a suspended component #25620
Merged
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
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 44c4e6f...e0790b4 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
acdlite
force-pushed
the
use-memo-async-function
branch
2 times, most recently
from
eab57ff
to
513f382
Compare
acdlite
commented
Nov 3, 2022
| case IndeterminateComponent: { | ||
| // Because it suspended with `use`, we can assume it's a | ||
| // function component. | ||
| unitOfWork.tag = FunctionComponent; |
There was a problem hiding this comment.
I thought this was a safe assumption but after thinking about it more, we probably need to add the new suspended work loop behavior to regular "throw a promise" Suspense, too, until that's fully deprecated. As of now, if you were to mix the two styles, you could fall into a loop because a component that throws a promise will cause a component that uses use to unwind and lose its resolved state.
And if that's the case, then we need to account for class components, because those are allowed to throw a promise.
There's also the case of suspending during reconciliation, though I think that will be special anyhow since we don't want to replay the parent component.
There was a problem hiding this comment.
Nvm, I thought about this more and I don't think the loop I described in the previous comment can happen.
acdlite
force-pushed
the
use-memo-async-function
branch
2 times, most recently
from
38c520b
to
1c59fbf
Compare
sebmarkbage
approved these changes
Nov 9, 2022
Before suspending, check if there are other pending updates that might possibly unblock the suspended component. If so, interrupt the current render and switch to working on that. This logic was already implemented for the old "throw a Promise" Suspense but has to be replicated for `use` because it suspends the work loop much earlier. I'm getting a little anxious about the divergence between the two Suspense patterns. I'm going to look into enabling the new behavior for the old pattern so that we can unify the implementations.
When replaying a suspended function components, we want to reuse the hooks that were computed during the original render. Currently we reset the state of the hooks right after the component suspends (or throws an error). This is too early because it doesn't give us an opportunity to wait for the promise to resolve. This refactors the work loop to reset the hooks right before unwinding instead of right after throwing. It doesn't include any other changes yet, so there should be no observable behavioral change.
Tiny refactor to refine the work loop variable so Flow knows it's not null when we access it in replaySuspendedUnitOfWork.
Currently, if you call setState in render, you must render the exact same hooks as during the first render pass. I'm about to add a behavior where if something suspends, we can reuse the hooks from the previous attempt. That means during initial render, if something suspends, we should be able to reuse the hooks that were already created and continue adding more after that. This will error in the current implementation because of the expectation that every render produces the same list of hooks. In this commit, I've changed the logic to allow more hooks to be added when replaying. But only during a mount — if there's already a current fiber, then the logic is unchanged, because we shouldn't add any additional hooks that aren't in the current fiber's list. Mounts are special because there's no current fiber to compare to. I haven't change any other behavior yet. The reason I've put this into its own step is there are a couple tests that intentionally break the Hook rule, to assert that React errors in these cases, and those happen to be coupled to the behavior. This is undefined behavior that is always accompanied by a warning and/or error. So the change should be safe.
When a component suspends, under some conditions, we can wait for the data to resolve and replay the component without unwinding the stack or showing a fallback in the interim. When we do this, we reuse the promises that were unwrapped during the previous attempts, so that if they aren't memoized, the result can still be used. We should do the same for all hooks. That way, if you _do_ memoize an async function call with useMemo, it won't be called again during the replay. This effectively gives you a local version of the functionality provided by `cache`, using the normal memoization patterns that have long existed in React.
Now that hook state is preserved while the work loop is suspended, we don't need to track the thenable state in the work loop. We can track it alongside the rest of the hook state. Before deleting the thenable state variable from the work loop, I need to remove the other places where it's referenced. One of them is `isThenableStateResolved`. This grabs the last thenable from the array and checks if it has resolved. This was a pointless indirection anyway. The thenable is already stored as `workInProgressThrownValue`. So we can check that directly.
Now that hook state is preserved while the work loop is suspended, we don't need to track the thenable state in the work loop. We can track it alongside the rest of the hook state. This is a nice simplification and also aligns better with how it works in Fizz and Flight. The promises will still be cleared when the component finishes rendering (either complete or unwind). In the future, we could stash the promises on the fiber and reuse them during an update. However, this would only work for `use` calls that occur before an prop/state/context is processed, because `use` calls can only be assumed to execute in the same order if no other props/state/context have changed. So it might not be worth doing until we have finer grained memoization.
acdlite
force-pushed
the
use-memo-async-function
branch
from
1c59fbf
to
44c4e6f
Compare
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.
Based on #25632
When a component suspends, under some conditions, we can wait for the data to resolve and replay the component without unwinding the stack or showing a fallback in the interim. When we do this, we reuse the promises that were unwrapped during the previous attempts, so that if they aren't memoized, the result can still be used.
We should do the same for all hooks. That way, if you do memoize an async function call with useMemo, it won't be called again during the replay. This effectively gives you a local version of the functionality provided by
cache, using the normal memoization patterns that have long existed in React.