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: useDeferredValue initialValue suspends forever without switching to final #27888

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jan 7, 2024

Fixes a bug in the experimental initialValue option for useDeferredValue (added in #27500).

If rendering the initialValue causes the tree to suspend, React should skip it and switch to rendering the final value instead. It should not wait for initialValue to resolve.

This is not just an optimization, because in some cases the initial value may never resolve — intentionally. For example, if the application does not provide an instant fallback state. This capability is, in fact, the primary motivation for the initialValue API.

I mostly implemented this correctly in the original PR, but I missed some cases where it wasn't working:

  • If there's no Suspense boundary between the useDeferredValue hook and the component that suspends, and we're not in the shell of the transition (i.e. there's a parent Suspense boundary wrapping the useDeferredValue hook), the deferred task would get incorrectly dropped.
  • Similarly, if there's no Suspense boundary between the useDeferredValue hook and the component that suspends, and we're rendering a synchronous update, the deferred task would get incorrectly dropped.

What these cases have in common is that it causes the useDeferredValue hook itself to be replaced by a Suspense fallback. The fix was the same for both. (It already worked in cases where there's no Suspense fallback at all, because those are handled differently, at the root.)

The way I discovered this was when investigating a particular bug in Next.js that would happen during a 'popstate' transition (back/forward), but not during a regular navigation. That's because we render popstate transitions synchronously to preserve browser's scroll position — which in this case triggered the second scenario above.

Fixes a bug in the experimental `initialValue` option for
`useDeferredValue` (added in facebook#27500).

If rendering the `initialValue` causes the tree to suspend, React should
skip it and switch to rendering the final value instead. It should not
wait for `initialValue` to resolve.

This is not just an optimization, because in some cases the initial
value may _never_ resolve — intentionally. For example, if the
application does not provide an instant fallback state. This capability
is, in fact, the primary motivation for the `initialValue` API.

I mostly implemented this correctly in the original PR, but I missed
some cases where it wasn't working:

- If there's no Suspense boundary between the `useDeferredValue` hook
and the component that suspends, and we're not in the shell of the
transition (i.e. there's a parent Suspense boundary wrapping the
`useDeferredValue` hook), the deferred task would get
incorrectly dropped.
- Similarly, if there's no Suspense boundary between the
`useDeferredValue` hook and the component that suspends, and we're
rendering a synchronous update, the deferred task would get incorrectly
dropped.

What these cases have in common is that it causes the `useDeferredValue`
hook itself to be replaced by a Suspense fallback. The fix was the same
for both. (It already worked in cases where there's no Suspense
fallback at all, because those are handled differently, at the root.)

The way I discovered this was when investigating a particular bug in
Next.js that would happen during a 'popstate' transition (back/forward),
but not during a regular navigation. That's because we render popstate
transitions synchronously to preserve browser's scroll position — which
in this case triggered the second scenario above.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 7, 2024
@react-sizebot
Copy link

Comparing: 1d5667a...61cde78

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 +0.11% 175.90 kB 176.11 kB +0.17% 54.76 kB 54.85 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.13% 177.97 kB 178.21 kB +0.16% 55.39 kB 55.49 kB
facebook-www/ReactDOM-prod.classic.js +0.26% 570.21 kB 571.71 kB +0.15% 100.35 kB 100.50 kB
facebook-www/ReactDOM-prod.modern.js +0.27% 554.06 kB 555.57 kB +0.18% 97.43 kB 97.61 kB
test_utils/ReactAllWarnings.js Deleted 67.55 kB 0.00 kB Deleted 16.54 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactART-prod.modern.js +0.43% 340.62 kB 342.10 kB +0.31% 57.79 kB 57.97 kB
react-native/implementations/ReactFabric-prod.fb.js +0.43% 328.96 kB 330.37 kB +0.34% 58.37 kB 58.57 kB
facebook-www/ReactART-prod.classic.js +0.42% 351.48 kB 352.96 kB +0.30% 59.67 kB 59.85 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js +0.42% 328.47 kB 329.85 kB +0.34% 57.73 kB 57.92 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js +0.42% 336.45 kB 337.86 kB +0.30% 59.79 kB 59.96 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js +0.41% 312.58 kB 313.88 kB +0.35% 55.29 kB 55.49 kB
react-native/implementations/ReactFabric-profiling.fb.js +0.40% 356.23 kB 357.65 kB +0.30% 62.52 kB 62.71 kB
react-native/implementations/ReactFabric-profiling.js +0.40% 341.02 kB 342.36 kB +0.31% 59.88 kB 60.06 kB
react-native/implementations/ReactFabric-prod.js +0.39% 321.62 kB 322.88 kB +0.34% 56.75 kB 56.94 kB
react-native/implementations/ReactNativeRenderer-profiling.fb.js +0.39% 363.76 kB 365.18 kB +0.28% 63.99 kB 64.17 kB
react-native/implementations/ReactNativeRenderer-profiling.js +0.39% 350.20 kB 351.55 kB +0.31% 61.63 kB 61.82 kB
react-native/implementations/ReactNativeRenderer-prod.js +0.38% 330.79 kB 332.05 kB +0.32% 58.48 kB 58.66 kB
oss-experimental/react-art/cjs/react-art.production.js +0.28% 599.92 kB 601.60 kB +0.26% 134.11 kB 134.46 kB
facebook-www/ReactDOM-prod.modern.js +0.27% 554.06 kB 555.57 kB +0.18% 97.43 kB 97.61 kB
facebook-www/ReactDOM-prod.classic.js +0.26% 570.21 kB 571.71 kB +0.15% 100.35 kB 100.50 kB
facebook-www/ReactDOMTesting-prod.modern.js +0.26% 570.47 kB 571.98 kB +0.17% 101.54 kB 101.71 kB
oss-stable-semver/react-art/cjs/react-art.production.js +0.26% 592.60 kB 594.16 kB +0.25% 132.39 kB 132.73 kB
oss-stable/react-art/cjs/react-art.production.js +0.26% 592.63 kB 594.19 kB +0.25% 132.42 kB 132.75 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.js +0.26% 605.26 kB 606.82 kB +0.23% 135.43 kB 135.75 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.js +0.26% 605.28 kB 606.84 kB +0.23% 135.46 kB 135.77 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.js +0.26% 606.69 kB 608.25 kB +0.23% 135.78 kB 136.10 kB
facebook-www/ReactDOMTesting-prod.classic.js +0.26% 585.02 kB 586.53 kB +0.14% 104.08 kB 104.23 kB
facebook-www/ReactDOM-profiling.modern.js +0.26% 584.49 kB 585.99 kB +0.16% 101.94 kB 102.10 kB
facebook-www/ReactDOM-profiling.classic.js +0.25% 600.70 kB 602.20 kB +0.14% 104.80 kB 104.95 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.js +0.24% 691.98 kB 693.66 kB +0.22% 151.01 kB 151.34 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.js +0.23% 733.18 kB 734.86 kB +0.23% 158.37 kB 158.73 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.js +0.23% 684.53 kB 686.09 kB +0.22% 149.34 kB 149.66 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.js +0.23% 684.55 kB 686.11 kB +0.21% 149.37 kB 149.69 kB
oss-experimental/react-art/cjs/react-art.production.min.js +0.22% 101.02 kB 101.24 kB +0.25% 30.99 kB 31.06 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.js +0.22% 725.73 kB 727.29 kB +0.22% 156.66 kB 157.01 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.js +0.22% 725.75 kB 727.32 kB +0.23% 156.69 kB 157.04 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js +0.20% 116.46 kB 116.70 kB +0.34% 35.98 kB 36.10 kB
test_utils/ReactAllWarnings.js Deleted 67.55 kB 0.00 kB Deleted 16.54 kB 0.00 kB

Generated by 🚫 dangerJS against 61cde78

@@ -41,6 +41,7 @@ export const StoreConsistency = /* */ 0b0000000000000100000000000000
// possible, because we're about to run out of bits.
export const ScheduleRetry = StoreConsistency;
export const ShouldSuspendCommit = Visibility;
export const DidDefer = ContentReset;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know how we use ContentReset but I assume you've considered that whether this ever propagates through subtree flags and that the reuse can't inadvertantly mix flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I picked one that wouldn’t conflict since we’re almost out of bits.

@acdlite acdlite merged commit f1039be into facebook:main Jan 8, 2024
36 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 8, 2024
… to final (#27888)

Fixes a bug in the experimental `initialValue` option for
`useDeferredValue` (added in #27500).

If rendering the `initialValue` causes the tree to suspend, React should
skip it and switch to rendering the final value instead. It should not
wait for `initialValue` to resolve.

This is not just an optimization, because in some cases the initial
value may _never_ resolve — intentionally. For example, if the
application does not provide an instant fallback state. This capability
is, in fact, the primary motivation for the `initialValue` API.

I mostly implemented this correctly in the original PR, but I missed
some cases where it wasn't working:

- If there's no Suspense boundary between the `useDeferredValue` hook
and the component that suspends, and we're not in the shell of the
transition (i.e. there's a parent Suspense boundary wrapping the
`useDeferredValue` hook), the deferred task would get incorrectly
dropped.
- Similarly, if there's no Suspense boundary between the
`useDeferredValue` hook and the component that suspends, and we're
rendering a synchronous update, the deferred task would get incorrectly
dropped.

What these cases have in common is that it causes the `useDeferredValue`
hook itself to be replaced by a Suspense fallback. The fix was the same
for both. (It already worked in cases where there's no Suspense fallback
at all, because those are handled differently, at the root.)

The way I discovered this was when investigating a particular bug in
Next.js that would happen during a 'popstate' transition (back/forward),
but not during a regular navigation. That's because we render popstate
transitions synchronously to preserve browser's scroll position — which
in this case triggered the second scenario above.

DiffTrain build for [f1039be](f1039be)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
… to final (facebook#27888)

Fixes a bug in the experimental `initialValue` option for
`useDeferredValue` (added in facebook#27500).

If rendering the `initialValue` causes the tree to suspend, React should
skip it and switch to rendering the final value instead. It should not
wait for `initialValue` to resolve.

This is not just an optimization, because in some cases the initial
value may _never_ resolve — intentionally. For example, if the
application does not provide an instant fallback state. This capability
is, in fact, the primary motivation for the `initialValue` API.

I mostly implemented this correctly in the original PR, but I missed
some cases where it wasn't working:

- If there's no Suspense boundary between the `useDeferredValue` hook
and the component that suspends, and we're not in the shell of the
transition (i.e. there's a parent Suspense boundary wrapping the
`useDeferredValue` hook), the deferred task would get incorrectly
dropped.
- Similarly, if there's no Suspense boundary between the
`useDeferredValue` hook and the component that suspends, and we're
rendering a synchronous update, the deferred task would get incorrectly
dropped.

What these cases have in common is that it causes the `useDeferredValue`
hook itself to be replaced by a Suspense fallback. The fix was the same
for both. (It already worked in cases where there's no Suspense fallback
at all, because those are handled differently, at the root.)

The way I discovered this was when investigating a particular bug in
Next.js that would happen during a 'popstate' transition (back/forward),
but not during a regular navigation. That's because we render popstate
transitions synchronously to preserve browser's scroll position — which
in this case triggered the second scenario above.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
… to final (#27888)

Fixes a bug in the experimental `initialValue` option for
`useDeferredValue` (added in #27500).

If rendering the `initialValue` causes the tree to suspend, React should
skip it and switch to rendering the final value instead. It should not
wait for `initialValue` to resolve.

This is not just an optimization, because in some cases the initial
value may _never_ resolve — intentionally. For example, if the
application does not provide an instant fallback state. This capability
is, in fact, the primary motivation for the `initialValue` API.

I mostly implemented this correctly in the original PR, but I missed
some cases where it wasn't working:

- If there's no Suspense boundary between the `useDeferredValue` hook
and the component that suspends, and we're not in the shell of the
transition (i.e. there's a parent Suspense boundary wrapping the
`useDeferredValue` hook), the deferred task would get incorrectly
dropped.
- Similarly, if there's no Suspense boundary between the
`useDeferredValue` hook and the component that suspends, and we're
rendering a synchronous update, the deferred task would get incorrectly
dropped.

What these cases have in common is that it causes the `useDeferredValue`
hook itself to be replaced by a Suspense fallback. The fix was the same
for both. (It already worked in cases where there's no Suspense fallback
at all, because those are handled differently, at the root.)

The way I discovered this was when investigating a particular bug in
Next.js that would happen during a 'popstate' transition (back/forward),
but not during a regular navigation. That's because we render popstate
transitions synchronously to preserve browser's scroll position — which
in this case triggered the second scenario above.

DiffTrain build for commit f1039be.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants