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

[RFC] Add initialValue option to useDeferredValue #24369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 13, 2022

Currently, useDeferredValue only works for updates. It will never during the initial render because there's no previous value to reuse. This means it can't be used to implement progressive enhancement.

This adds an optional initialValue argument to useDeferredValue. When provided, the initial mount will use initialValue if it's during an urgent render. Otherwise it will use the latest, canonical value.

During server rendering and hydration, it will always use the initialValue instead of the canonical value, regardless of priority, to avoid a hydration mismatch.

The name "initial value" isn't ideal because during a non-urgent client render, it's disregarded entirely. It's more like a "lightweight" value that will later be upgraded to a "heavier" one. Needs some bikeshedding.

When initialValue is omitted, the behavior is the same as today.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 13, 2022
@sizebot
Copy link

sizebot commented Apr 13, 2022

Comparing: bd08137...b441be7

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.09% 131.53 kB 131.66 kB +0.09% 42.11 kB 42.15 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.09% 136.80 kB 136.93 kB +0.11% 43.68 kB 43.73 kB
facebook-www/ReactDOM-prod.classic.js +0.11% 441.05 kB 441.56 kB +0.06% 80.44 kB 80.49 kB
facebook-www/ReactDOM-prod.modern.js +0.12% 426.30 kB 426.81 kB +0.07% 78.26 kB 78.31 kB
facebook-www/ReactDOMForked-prod.classic.js +0.11% 441.05 kB 441.56 kB +0.06% 80.44 kB 80.49 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against b441be7

@gaearon
Copy link
Collaborator

gaearon commented Apr 13, 2022

What do you mean by progressive enhancement?

@sebmarkbage
Copy link
Collaborator

Minor nit that the initial value can't be the value undefined. Slight foot gun.

Last time I thought about in context of Retry I convinced me that it would eventually be ok only because it wouldn't apply to initial render.

There's something weird about deferring the value when the thing is not visible. E.g. because it resuspended and then gets rendered again or when something is initially rendered as Offscreen. Deferring it would also deprioritize that update.

You couldn't implement this in user space since the effect wouldn't fire while Offscreen to allow the second render pass. So you'd always see the initial value and then toggle it. That might be a reason for why it should be built-in but I'm not sure it makes sense to make that a two pass render rather than just going straight for the end value if that's what you're going to end up likely seeing anyway. At least first and then only fallback if that suspends.

I'm also curious what you mean by progressive enhancement because for the SSR case, and the use case for the isSSR flag, is to avoid two pass renders when it's already rendered on the client. You only want the first pass when it's server rendered as a fallback but when it's client rendered from the beginning you don't want that.

Even if it's not SSR and you want to have a lighter implementation and then React.lazy load a second implementation, I think you would want to always try the better implementation first in case it's already loaded instead of always rendering the lighter implementation and then switching.

@sebmarkbage
Copy link
Collaborator

Maybe we need a new hook like let value = useProgressiveValue(heavyValue, lightValue); that always uses the second value for SSR/hydration. When it's rendered in a sync priority on the client (either initial render or update) then it uses the second value. Then it does an upgrade to the first value in a transition. If it's rendered or updated in a transition/retry it tries the first value first, and if that suspends, it captures and rerenders with the second value.

@acdlite
Copy link
Collaborator Author

acdlite commented Apr 19, 2022

the use case for the isSSR flag, is to avoid two pass renders when it's already rendered on the client.

Yeah that's true, the only reason I made it use the "light" value during initial render regardless of priority is to avoid a server-client mismatch, but first it should check if it's actually hydrating, too.

Maybe we need a new hook like let value = useProgressiveValue(heavyValue, lightValue); that always uses the second value for SSR/hydration. When it's rendered in a sync priority on the client (either initial render or update) then it uses the second value.

The first part makes sense, but why would it switch to the "light" value during an update? Seems like it's better to stay on the previous value. That's why I figured these should be the same hook, since except for server rendering the behavior is the same.

Based on this feedback I think my preference is to add an isHydrating check to the client implementation, so that if it's not hydrating and the priority is not sync, it uses the canonical one. But keep the rest the same.

@sebmarkbage
Copy link
Collaborator

What is the canonical one? The first one? So the second value is only for SSR/hydration or sync updates?

@acdlite
Copy link
Collaborator Author

acdlite commented Apr 19, 2022

What is the canonical one? The first one? So the second value is only for SSR/hydration or sync updates?

Yeah to me it's similar to passing an initial value to useState. Only matters for initial render.

@sebmarkbage
Copy link
Collaborator

It's a little weird that it wouldn't be used for transitions but I think that makes sense but probably needs a different name.

@acdlite
Copy link
Collaborator Author

acdlite commented Apr 19, 2022

Minor nit that the initial value can't be the value undefined. Slight foot gun.

Yeah we could warn in dev if there's a second argument but it's undefined. As opposed to a missing argument. I assume that's expressible in TypeScript.

It's a little weird that it wouldn't be used for transitions but I think that makes sense but probably needs a different name.

Progressive value?

Currently, useDeferredValue only works for updates. It will never during
the initial render because there's no previous value to reuse. This
means it can't be used to implement progressive enhancement.

This adds an optional initialValue argument to useDeferredValue. When
provided, the initial mount will use initialValue if it's during an
urgent render. Otherwise it will use the latest, canonical value.

During server rendering and hydration, it will always use the
initialValue instead of the canonical value, regardless of priority, to
avoid a hydration mismatch.

The name "initial value" isn't ideal because during a non-urgent client
render, it's disregarded entirely. It's more like a "lightweight" value
that will later be upgraded to a "heavier" one. Needs some bikeshedding.

When initialValue is omitted, the behavior is the same as today.
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.

None yet

5 participants