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

[React 18] Selective Hydration fails hydration when using context api #22692

Open
maraisr opened this issue Nov 4, 2021 · 20 comments
Open

[React 18] Selective Hydration fails hydration when using context api #22692

maraisr opened this issue Nov 4, 2021 · 20 comments
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion

Comments

@maraisr
Copy link

maraisr commented Nov 4, 2021

Based on the selective hydration example provided by @gaearon ??

https://codesandbox.io/s/mystifying-haibt-39oed

Look out for πŸ‘‰
image

One can see that hydrating html does in fact fail when using Context API β€” or perhaps that's a red herring? The trees do in fact line up during render, and hydration, there's just an effect that sets state β€” but that is post hydration??

Could be them related; in anycase β€” strictly speaking to that code sandbox, what am i doing wrongly?


Initially caught when ImportedComponent fails see https://github.com/theKashey/react-imported-component/blob/c290d76623693389a11cc514c92f47efadac47ba/src/ui/ImportedController.tsx#L29

@maraisr maraisr added React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion labels Nov 4, 2021
@maraisr
Copy link
Author

maraisr commented Nov 4, 2021

cc @theKashey

@theKashey
Copy link
Contributor

As I understand - any context update invalidates all dehydrated SuspenseComponents. Not sure is it a feature or a bug, and is there any extra flag on Suspense to control this behavior.

@maraisr
Copy link
Author

maraisr commented Nov 4, 2021

Perhaps an EventHorizon aka memo we can wrap and all is good. As context update not necessarily altering the tree south a Suspense boundary.

Think really this means any state update until a Suspended hydrated component, will cause things to break β€” not really about context.

@salazarm
Copy link
Contributor

salazarm commented Nov 4, 2021

So its unrelated to context. I setup a more minimal repro here: https://codesandbox.io/s/crimson-frost-y5tz9?file=/src/App.js

So what happens is on initial hydration when the component suspends we don't switch to the fallback, but then later when we re-render App we see the Sidebar component suspended and we do decide to show the fallback then. I think maybe we should skip over dehydrated suspense boundaries during re-render. cc @sebmarkbage @acdlite

@sebmarkbage
Copy link
Collaborator

It’s a feature. If something doesn’t update, then sure, it’s unnecessary but we don’t know that. If something does update, then the tree would be inconsistent and perhaps non-sensical. Eg imagine switching selected item and it doesn’t change the selection and then you press the delete button.

Note that this doesn’t (shouldn’t) happen with transitions since it’ll first go back in time and then try to hydrate again. If it suspends, it’s just a transition so it waits to commit until it unsuspends.

It usually isn’t noticeable but becomes noticeable when you use other bad patterns like sync rerenders in a layout effect. They’re common but still bad for perf and should be avoided.

It used to happen less because more things were async but since we made things sync, they happen more now.

The issue with sync updates is that we can’t delay the update or give up consistency. So we have to show the fallback.

However we might throw away the SSR content unnecessarily. It’s tricky because sync updates might rely on legacy things that read mutable state so you can’t necessarily safely do a two pass render. Maybe certain things like updates in passive effects or pseudo-sync things like Default pri, could be made to do two pass.

But in general, the way to avoid it is by using a transition.

@sebmarkbage
Copy link
Collaborator

When you don’t use Context, the trick is to use memo or useMemo.

e.g.

useMemo(() => <Suspense…>…</Suspense>)

That way we know it hasn’t changed. Context is brute force because we don’t know if something below reads it.

@theKashey
Copy link
Contributor

If I read correctly - there is no way to keep areas suspended with context updates, many of them can have just "not-UI" nature, like hydration/loading state to power phased code splitting (importForInteractions)

Can I ask to provide more control over this to the user space, as I am more than capable to invalidate my caches and can understand when I need suspended fragment to be hydrated. More of it - probably in this case I need it to be actually "hydrated", ie want hydration process to continue, not destroy existing tree and display fallback.

@sebmarkbage
Copy link
Collaborator

The way around it is to not use React for tracking the updates. You can use a subscription (possibly with useSyncExternalStore).

If you want deep advanced control over how parts of a page is hydrated, you’re better off splitting up into multiple roots. Like is already possible. It’s going to interplay with other features in subtle ways anyway that you’ll need to resolve.

@theKashey
Copy link
Contributor

That sounds like a really complication for any existing applications, which can be summarised as β€œuse a self contained state management solution, some of them will work, not low level system primitives”

Multiple roots are falling into the same bucket, as every such root should be wrapped with all required providers with common state or synchronised, or exists outside or React.

For quite a while the overall diffraction was to put more stuff β€œin”, so asking to put some β€œout” is not an option as it requires some sort of shotgun surgery for many third party dependencies

@gaearon
Copy link
Collaborator

gaearon commented Nov 6, 2021

To be clear, you do have a level of control there β€” updates wrapped in startTransition won't do this.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Nov 6, 2021

The general model here is that code is expected to load relatively quickly and that you eagerly hydrate the whole page on idle.

This makes it so that you typically won’t hit the scenario of hiding existing content. Since you’ll have downloaded everything by the time the user interacts with the page.

The exception is things that update immediately and synchronously without user interaction - which is bad practice for other reasons already.

The other exception when this can become a problem is when you don’t subscribe to the eager hydration model. Eg if you only want to even start downloading the code upon interaction. That’s a very different model than what we’ve designed for, so it requires a different design. That’s the case where I suggest splitting into multiple roots.

Another case if you do clever stuff by inferring that hydration is going on but I think it’s fair that you have to spend some time updating those libraries when the hydration model has drastically changed. It’s likely to break anyway since the assumptions have changed.

In other cases this wouldn’t be an issue.

@theKashey
Copy link
Contributor

theKashey commented Nov 7, 2021

β€˜startTrasition’ will not work as it will delay the state update until it is β€œpossible” - https://codesandbox.io/s/crimson-breeze-17c8n?file=/src/App.js

hydrate the whole page on idle

why would I want to hydrate whole page, when it’s footer might be never viewed by the user, as well as many other elements (SVG) can be kept β€œdead html” for quite a while.

I was looking forward to hydrate-on-interaction or hydrate-on-scroll, but not on β€œjust hydrate”

@gaearon
Copy link
Collaborator

gaearon commented Nov 7, 2021

why would I want to hydrate whole page, when it’s footer might be never viewed by the user, as well as many other elements (SVG) can be kept β€œdead html” for quite a while.

Because there is no reason not to. Since the algorithm is interruptible and each individual component shouldn't stall the thread in isolation, doing it eagerly is not prohibitive. It's just an optimization.

@maraisr
Copy link
Author

maraisr commented Nov 7, 2021

For websites though; you want to run as less code as possible to keep Google happy.

Time to Interactive isn't about when you've loaded, but rather when no code is running. Afaik. But then again, Input Delay could also be diress if hydration happens on interaction.

For apps that's different, as elements on the screen need to be interactive asap (like popping open a chat window).

So really it's about, do you optimise for the human (as reacts done), or do you optimise for Google.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Nov 7, 2021

Time to interactive is when no JS is blocking. Old school hydration would be blocking but concurrent rendering is not since it yields.

If you never want to hydrate, there’s Server Components. If you hydrate on interaction, then you risk having a poor interaction performance when it’s needed. So warming up is beneficial.

@theKashey
Copy link
Contributor

Warming: doing hydration 1) on scroll 2) on mouse move 3) on touchstart/mousedown. Given a very limited scope to hydrate no real delay should be experienced by the user. However, while every small boundary might not consume much time, so no one will benefit from gain this time back - having a dozen such areas on the page can mitigate a death by a thousand cuts - it will be fewer cuts.

Server Components are a good option, but currently the main goal is to optimize the first impression, when too many different scripts are initializing and fighting for the browser time. On the sub-navigation no analytics, trackers, or any other third-party scripts will interfere and the application naturally has more performance budget.

In the old world, we had suppressHydrationWarning, is it possible to have something like it in the new? For context we had observedBits, is it possible to mark Context as "non-invasive"? There was a problem in React 15 with PureComponents preventing any update propagation, sometimes I do miss this feature (freeze) in modern React.

Actually, the question - how one can build something alike Server Components in the userspace? Everything above sounds like a part of this bigger picture.

@sebmarkbage
Copy link
Collaborator

Any mode we add undermines the composability elsewhere so we generally don’t want to add options like that. As I see it the whole point of React is to create an opinionated protocol between components.

Similarly there’s no option to not hydrate a subtree since that would mean that a component like an auto-playing video can’t rely on hydration to start and control video playback for example. Meaning that we’re somewhat opinionated that everything that’s a β€œclient component” should be able to rely on being hydrated relatively quickly.

It would be good to be able to talk more concretely about a real page/scenario to understand when this would be an issue and if there are solutions to that problem by changing something else on the page. Intuition can fail us otherwise.

Note that there is still the option to reprioritize what gets hydrated first. React does it automatically on hover. You can add custom hints with unstable_scheduleHydration. So even in contention you have the option to do cpu work only if nothing else is prioritized. This doesn’t matter for this scenario though since it should only kick in if the bytes hasn’t loaded on network (if the environment is set up correctly).

Contention is more likely to be a problem for network since if you flood the network, it’s difficult to fetch on demand. The issue is that if your latency is too high so if you request it on demand, then your page won’t be interactive quickly enough. So you want to preload it. If you can preload quickly enough then you probably shouldn’t be server rendering that much content to begin with because the magic trick is going to be ruined. So you’re better off holding on showing it. If the latency is low enough with hints like scroll/hover, then this issue generally doesn’t happen anyway.

But again, it would be good to see if this actually happens when all our other recommendations are in place.

@theKashey
Copy link
Contributor

A little update on the issue - updating Context in useLayoutEffect leads to a Suspense disposal. useEffect works fine.

Demo: https://codesandbox.io/s/competent-sound-rv3ms?file=/src/App.js:882-886

@sebmarkbage
Copy link
Collaborator

@theKashey Memoizing the App or anything along the way to the Suspense boundary also fixes it.

However, I think this is a bug because the Suspense boundary isn't actually suspended. This case should only happen if it's suspended.

@jon1234567
Copy link

(paint)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion
Projects
None yet
Development

No branches or pull requests

6 participants