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

Remove recoverable error when a sync update flows into a dehydrated boundary #25692

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Nov 16, 2022

This just removes the error but the underlying issue is still there, and it's likely that the best course of action is to not update in effects and to wrap most updates in startTransition. However, that's more of a performance concern which is not something we generally do even in recoverable errors since they're less actionable and likely belong in another channel. It is also likely that in many cases this happens so rarely because you have to interact quickly enough that it can often be ignored.

After changes to other parts of the model, this only happens for sync/discrete updates. There are three scenarios that can happen:

  • We replace a server rendered fallback with a client rendered fallback. Other than this potentially causing some flickering in the loading state, it's not a big deal.
  • We replace the server rendered content with a client side fallback if this suspends on the client. This is in line with what would happen anyway. We will loose state of forms which is not intended semantics. State and animations etc would've been lost anyway if it was client-side so that's not a concern.
  • We replace the server rendered content with a client side rendered tree and lose selection/state and form state. While the content looks the same, which is unfortunate.

In most scenarios it's a bad loading state but it's the same scenario as flushing sync client-side. So it's not so bad.

The big change here is that we consider this a bug of React that we should fix. Therefore it's not actionable to users today because it should just get fixed. So we're removing the error early. Although anyone that has fixed these issues already are probably better off for it.

To fix this while still hydrating we need to be able to rewind a sync tree and then replay it.

@tyao1 is going to add a Sync hydration lane. This is will allow us to rewind the tree when we hit this state, and replay it given the previous Context, hydrate and then reapply the update. The reason we didn't do this originally is because it causes sync mode to unwind where as for backwards compatibility we didn't want to cause that breaking semantic - outside Suspense boundaries - and we don't want that semantic longer term. We're only do this as a short term fix.

We should also have a way to leave a partial tree in place. If the sync hydration lane suspends, we should be able to switch to a client side fallback without throwing away the state of the DOM and then hydrate later.

We now know how we want to fix this longer term. We're going to move all Contexts into resumable trees like what Fizz/Flight does. That way we can leave the original Context at the hydration boundaries and then resume from there. That way the rewinding would never happen even in the existence of a sync hydration lane which would only apply locally to the dehydrated tree.

So the steps are 1) remove the error 2) add the sync hydration lane with rewinding 3) Allow hiding server-rendered content while still not hydrated 4) add resumable contexts at these boundaries.

Fixes #25625 and #24959.

@sizebot
Copy link

sizebot commented Nov 16, 2022

Comparing: d1e35c7...2157c76

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 153.65 kB 153.64 kB = 48.90 kB 48.90 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 155.57 kB 155.56 kB = 49.51 kB 49.51 kB
facebook-www/ReactDOM-prod.classic.js = 530.48 kB 530.41 kB = 94.68 kB 94.67 kB
facebook-www/ReactDOM-prod.modern.js = 515.74 kB 515.66 kB = 92.50 kB 92.49 kB
facebook-www/ReactDOMForked-prod.classic.js = 530.48 kB 530.41 kB = 94.68 kB 94.67 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js = 99.62 kB 99.39 kB = 30.67 kB 30.58 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.min.js = 99.56 kB 99.33 kB = 30.63 kB 30.54 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.min.js = 99.54 kB 99.30 kB = 30.63 kB 30.54 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js = 99.86 kB 99.62 kB = 31.10 kB 30.98 kB
oss-stable/react-test-renderer/umd/react-test-renderer.production.min.js = 99.80 kB 99.56 kB = 31.07 kB 30.97 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.production.min.js = 99.77 kB 99.54 kB = 31.07 kB 30.97 kB

Generated by 🚫 dangerJS against 2157c76

@sebmarkbage sebmarkbage merged commit e1dd0a2 into facebook:main Nov 16, 2022
@tyao1 tyao1 mentioned this pull request Nov 16, 2022
mofeiZ pushed a commit to mofeiZ/react that referenced this pull request Nov 17, 2022
…oundary (facebook#25692)

This just removes the error but the underlying issue is still there, and
it's likely that the best course of action is to not update in effects
and to wrap most updates in startTransition. However, that's more of a
performance concern which is not something we generally do even in
recoverable errors since they're less actionable and likely belong in
another channel. It is also likely that in many cases this happens so
rarely because you have to interact quickly enough that it can often be
ignored.

After changes to other parts of the model, this only happens for
sync/discrete updates. There are three scenarios that can happen:
- We replace a server rendered fallback with a client rendered fallback.
Other than this potentially causing some flickering in the loading
state, it's not a big deal.
- We replace the server rendered content with a client side fallback if
this suspends on the client. This is in line with what would happen
anyway. We will loose state of forms which is not intended semantics.
State and animations etc would've been lost anyway if it was client-side
so that's not a concern.
- We replace the server rendered content with a client side rendered
tree and lose selection/state and form state. While the content looks
the same, which is unfortunate.

In most scenarios it's a bad loading state but it's the same scenario as
flushing sync client-side. So it's not so bad.

The big change here is that we consider this a bug of React that we
should fix. Therefore it's not actionable to users today because it
should just get fixed. So we're removing the error early. Although
anyone that has fixed these issues already are probably better off for
it.

To fix this while still hydrating we need to be able to rewind a sync
tree and then replay it.

@tyao1 is going to add a Sync hydration lane. This is will allow us to
rewind the tree when we hit this state, and replay it given the previous
Context, hydrate and then reapply the update. The reason we didn't do
this originally is because it causes sync mode to unwind where as for
backwards compatibility we didn't want to cause that breaking semantic -
outside Suspense boundaries - and we don't want that semantic longer
term. We're only do this as a short term fix.

We should also have a way to leave a partial tree in place. If the sync
hydration lane suspends, we should be able to switch to a client side
fallback without throwing away the state of the DOM and then hydrate
later.

We now know how we want to fix this longer term. We're going to move all
Contexts into resumable trees like what Fizz/Flight does. That way we
can leave the original Context at the hydration boundaries and then
resume from there. That way the rewinding would never happen even in the
existence of a sync hydration lane which would only apply locally to the
dehydrated tree.

So the steps are 1) remove the error 2) add the sync hydration lane with
rewinding 3) Allow hiding server-rendered content while still not
hydrated 4) add resumable contexts at these boundaries.

Fixes facebook#25625 and facebook#24959.
tyao1 added a commit that referenced this pull request Nov 17, 2022
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
For more context: #25692

Based on #25695. This PR adds the
`SyncHydrationLane` so we rewind on sync updates during selective
hydration. Also added tests for ContinuouseHydration and
DefaultHydration lanes.


## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->
yarn test
rickhanlonii pushed a commit that referenced this pull request Dec 3, 2022
…oundary (#25692)

This just removes the error but the underlying issue is still there, and
it's likely that the best course of action is to not update in effects
and to wrap most updates in startTransition. However, that's more of a
performance concern which is not something we generally do even in
recoverable errors since they're less actionable and likely belong in
another channel. It is also likely that in many cases this happens so
rarely because you have to interact quickly enough that it can often be
ignored.

After changes to other parts of the model, this only happens for
sync/discrete updates. There are three scenarios that can happen:
- We replace a server rendered fallback with a client rendered fallback.
Other than this potentially causing some flickering in the loading
state, it's not a big deal.
- We replace the server rendered content with a client side fallback if
this suspends on the client. This is in line with what would happen
anyway. We will loose state of forms which is not intended semantics.
State and animations etc would've been lost anyway if it was client-side
so that's not a concern.
- We replace the server rendered content with a client side rendered
tree and lose selection/state and form state. While the content looks
the same, which is unfortunate.

In most scenarios it's a bad loading state but it's the same scenario as
flushing sync client-side. So it's not so bad.

The big change here is that we consider this a bug of React that we
should fix. Therefore it's not actionable to users today because it
should just get fixed. So we're removing the error early. Although
anyone that has fixed these issues already are probably better off for
it.

To fix this while still hydrating we need to be able to rewind a sync
tree and then replay it.

@tyao1 is going to add a Sync hydration lane. This is will allow us to
rewind the tree when we hit this state, and replay it given the previous
Context, hydrate and then reapply the update. The reason we didn't do
this originally is because it causes sync mode to unwind where as for
backwards compatibility we didn't want to cause that breaking semantic -
outside Suspense boundaries - and we don't want that semantic longer
term. We're only do this as a short term fix.

We should also have a way to leave a partial tree in place. If the sync
hydration lane suspends, we should be able to switch to a client side
fallback without throwing away the state of the DOM and then hydrate
later.

We now know how we want to fix this longer term. We're going to move all
Contexts into resumable trees like what Fizz/Flight does. That way we
can leave the original Context at the hydration boundaries and then
resume from there. That way the rewinding would never happen even in the
existence of a sync hydration lane which would only apply locally to the
dehydrated tree.

So the steps are 1) remove the error 2) add the sync hydration lane with
rewinding 3) Allow hiding server-rendered content while still not
hydrated 4) add resumable contexts at these boundaries.

Fixes #25625 and #24959.
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jan 30, 2023
Summary:
Three problems popped up during the sync:
- facebook/react@07f46ecf2 breaks breaks tests
- facebook/react@6fb8133ed breaks fbsource tests. I added a workaround and created a test for the team that owns the test.
- https://fb.workplace.com/groups/flowlang/permalink/1198137807458547/ enables local type interference in fbsource but not in github React repo and some code breaks. Addressed in facebook/react#26064

This sync includes the following changes:
- **[17f6912a4](facebook/react@17f6912a4 )**: Add flow types to ReactFiberHooks ([#25752](facebook/react#25752)) //<Samuel Susla>//
- **[f101c2d0d](facebook/react@f101c2d0d )**: Remove Reconciler fork (2/2) ([#25775](facebook/react#25775)) //<Jan Kassens>//
- **[420f0b7fa](facebook/react@420f0b7fa )**: Remove Reconciler fork (1/2) ([#25774](facebook/react#25774)) //<Jan Kassens>//
- **[3ba7add60](facebook/react@3ba7add60 )**: Allow async blocks in `to(Error|Warn)Dev` ([#25338](facebook/react#25338)) //<Sebastian Silbermann>//
- **[fa11bd6ec](facebook/react@fa11bd6ec )**: [ServerRenderer] Add option to send instructions as data attributes ([#25437](facebook/react#25437)) //<mofeiZ>//
- **[e98225485](facebook/react@e98225485 )**: Add ref cleanup function ([#25686](facebook/react#25686)) //<Samuel Susla>//
- **[15557fa67](facebook/react@15557fa67 )**: [Fix] properly track `useId` use in StrictMode in development ([#25713](facebook/react#25713)) //<Josh Story>//
- **[8a23def32](facebook/react@8a23def32 )**: Resubmit Add HydrationSyncLane ([#25711](facebook/react#25711)) //<Tianyu Yao>//
- **[2655c9354](facebook/react@2655c9354 )**: Fizz Browser: fix precomputed chunk being cleared on Node 18 ([#25645](facebook/react#25645)) //<Jimmy Lai>//
- **[c08d8b804](facebook/react@c08d8b804 )**: Revert "Add SyncHydrationLane" ([#25708](facebook/react#25708)) //<Tianyu Yao>//
- **[56ffca8b9](facebook/react@56ffca8b9 )**: Add Bun streaming server renderer ([#25597](facebook/react#25597)) //<Colin McDonnell>//
- **[f31005d6a](facebook/react@f31005d6a )**: Add SyncHydrationLane ([#25698](facebook/react#25698)) //<Tianyu Yao>//
- **[f284d9faf](facebook/react@f284d9faf )**: Track ThenableState alongside other hooks //<Andrew Clark>//
- **[6b4c0314e](facebook/react@6b4c0314e )**: Check thenable instead of thenableState //<Andrew Clark>//
- **[33e3d2878](facebook/react@33e3d2878 )**: Reuse hooks when replaying a suspended component //<Andrew Clark>//
- **[4387d752d](facebook/react@4387d752d )**: Allow more hooks to be added when replaying mount //<Andrew Clark>//
- **[5eb78d0a0](facebook/react@5eb78d0a0 )**: Pass ThenableState to replaySuspendedUnitOfWork //<Andrew Clark>//
- **[4a2d86bdd](facebook/react@4a2d86bdd )**: Don't reset work loop until stack is unwound //<Andrew Clark>//
- **[9dfbd9fa9](facebook/react@9dfbd9fa9 )**: use: Don't suspend if there are pending updates //<Andrew Clark>//
- **[44c4e6f4d](facebook/react@44c4e6f4d )**: Force unwind work loop during selective hydration ([#25695](facebook/react#25695)) //<Andrew Clark>//
- **[7b17f7bbf](facebook/react@7b17f7bbf )**: Enable warning for defaultProps on function components for everyone ([#25699](facebook/react#25699)) //<Sebastian Markbåge>//
- **[6fb8133ed](facebook/react@6fb8133ed )**: Turn on string ref deprecation warning for everybody (not codemoddable) ([#25383](facebook/react#25383)) //<Sebastian Silbermann>//
- **[07f46ecf2](facebook/react@07f46ecf2 )**: Turn on key spread warning in jsx-runtime for everyone ([#25697](facebook/react#25697)) //<Sebastian Markbåge>//
- **[d65b88d03](facebook/react@d65b88d03 )**: Eagerly initialize an mutable object for instance.refs ([#25696](facebook/react#25696)) //<Sebastian Markbåge>//
- **[c343f8025](facebook/react@c343f8025 )**: [react-float] feature detect getRootNode ([#25689](facebook/react#25689)) //<Jan Kassens>//
- **[e1dd0a2f5](facebook/react@e1dd0a2f5 )**: Remove recoverable error when a sync update flows into a dehydrated boundary ([#25692](facebook/react#25692)) //<Sebastian Markbåge>//
- **[c54e3541b](facebook/react@c54e3541b )**: [DevTools] bug fix for Hydrating fibers ([#25663](facebook/react#25663)) //<Mengdi Chen>//

Changelog:
[General][Changed] - React Native sync for revisions d1e35c7...17f6912

jest_e2e[run_all_tests]

Reviewed By: makovkastar

Differential Revision: D42804802

fbshipit-source-id: 6a9f00724cc73378025bbd04edb2d17760a87280
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Three problems popped up during the sync:
- facebook/react@07f46ecf2 breaks breaks tests
- facebook/react@6fb8133ed breaks fbsource tests. I added a workaround and created a test for the team that owns the test.
- https://fb.workplace.com/groups/flowlang/permalink/1198137807458547/ enables local type interference in fbsource but not in github React repo and some code breaks. Addressed in facebook/react#26064

This sync includes the following changes:
- **[17f6912a4](facebook/react@17f6912a4 )**: Add flow types to ReactFiberHooks ([facebook#25752](facebook/react#25752)) //<Samuel Susla>//
- **[f101c2d0d](facebook/react@f101c2d0d )**: Remove Reconciler fork (2/2) ([facebook#25775](facebook/react#25775)) //<Jan Kassens>//
- **[420f0b7fa](facebook/react@420f0b7fa )**: Remove Reconciler fork (1/2) ([facebook#25774](facebook/react#25774)) //<Jan Kassens>//
- **[3ba7add60](facebook/react@3ba7add60 )**: Allow async blocks in `to(Error|Warn)Dev` ([facebook#25338](facebook/react#25338)) //<Sebastian Silbermann>//
- **[fa11bd6ec](facebook/react@fa11bd6ec )**: [ServerRenderer] Add option to send instructions as data attributes ([facebook#25437](facebook/react#25437)) //<mofeiZ>//
- **[e98225485](facebook/react@e98225485 )**: Add ref cleanup function ([facebook#25686](facebook/react#25686)) //<Samuel Susla>//
- **[15557fa67](facebook/react@15557fa67 )**: [Fix] properly track `useId` use in StrictMode in development ([facebook#25713](facebook/react#25713)) //<Josh Story>//
- **[8a23def32](facebook/react@8a23def32 )**: Resubmit Add HydrationSyncLane ([facebook#25711](facebook/react#25711)) //<Tianyu Yao>//
- **[2655c9354](facebook/react@2655c9354 )**: Fizz Browser: fix precomputed chunk being cleared on Node 18 ([facebook#25645](facebook/react#25645)) //<Jimmy Lai>//
- **[c08d8b804](facebook/react@c08d8b804 )**: Revert "Add SyncHydrationLane" ([facebook#25708](facebook/react#25708)) //<Tianyu Yao>//
- **[56ffca8b9](facebook/react@56ffca8b9 )**: Add Bun streaming server renderer ([facebook#25597](facebook/react#25597)) //<Colin McDonnell>//
- **[f31005d6a](facebook/react@f31005d6a )**: Add SyncHydrationLane ([facebook#25698](facebook/react#25698)) //<Tianyu Yao>//
- **[f284d9faf](facebook/react@f284d9faf )**: Track ThenableState alongside other hooks //<Andrew Clark>//
- **[6b4c0314e](facebook/react@6b4c0314e )**: Check thenable instead of thenableState //<Andrew Clark>//
- **[33e3d2878](facebook/react@33e3d2878 )**: Reuse hooks when replaying a suspended component //<Andrew Clark>//
- **[4387d752d](facebook/react@4387d752d )**: Allow more hooks to be added when replaying mount //<Andrew Clark>//
- **[5eb78d0a0](facebook/react@5eb78d0a0 )**: Pass ThenableState to replaySuspendedUnitOfWork //<Andrew Clark>//
- **[4a2d86bdd](facebook/react@4a2d86bdd )**: Don't reset work loop until stack is unwound //<Andrew Clark>//
- **[9dfbd9fa9](facebook/react@9dfbd9fa9 )**: use: Don't suspend if there are pending updates //<Andrew Clark>//
- **[44c4e6f4d](facebook/react@44c4e6f4d )**: Force unwind work loop during selective hydration ([facebook#25695](facebook/react#25695)) //<Andrew Clark>//
- **[7b17f7bbf](facebook/react@7b17f7bbf )**: Enable warning for defaultProps on function components for everyone ([facebook#25699](facebook/react#25699)) //<Sebastian Markbåge>//
- **[6fb8133ed](facebook/react@6fb8133ed )**: Turn on string ref deprecation warning for everybody (not codemoddable) ([facebook#25383](facebook/react#25383)) //<Sebastian Silbermann>//
- **[07f46ecf2](facebook/react@07f46ecf2 )**: Turn on key spread warning in jsx-runtime for everyone ([facebook#25697](facebook/react#25697)) //<Sebastian Markbåge>//
- **[d65b88d03](facebook/react@d65b88d03 )**: Eagerly initialize an mutable object for instance.refs ([facebook#25696](facebook/react#25696)) //<Sebastian Markbåge>//
- **[c343f8025](facebook/react@c343f8025 )**: [react-float] feature detect getRootNode ([facebook#25689](facebook/react#25689)) //<Jan Kassens>//
- **[e1dd0a2f5](facebook/react@e1dd0a2f5 )**: Remove recoverable error when a sync update flows into a dehydrated boundary ([facebook#25692](facebook/react#25692)) //<Sebastian Markbåge>//
- **[c54e3541b](facebook/react@c54e3541b )**: [DevTools] bug fix for Hydrating fibers ([facebook#25663](facebook/react#25663)) //<Mengdi Chen>//

Changelog:
[General][Changed] - React Native sync for revisions d1e35c7...17f6912

jest_e2e[run_all_tests]

Reviewed By: makovkastar

Differential Revision: D42804802

fbshipit-source-id: 6a9f00724cc73378025bbd04edb2d17760a87280
@OliverJAsh
Copy link

Has this change been released yet? Or is it part of the forthcoming v19?

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

Successfully merging this pull request may close these issues.

Update before hydration completed error does not clarify which suspense boundary or which update
5 participants