GoogleChromeLabs / react-adaptive-hooks

Deliver experiences best suited to a user's device and network constraints
Apache License 2.0
5.11k stars 114 forks source link

React Adaptive Hooks will cause attribute mismatch during server side rendering #18

Open gja opened 4 years ago

gja commented 4 years ago

Hi. I saw a recent issue #2 regarding server side rendering. However, this behaviour simply hides the attribute mismatch that can happen due to the SSR phase and the client phase rendering differently.

I have built an example repo that showcases this mismatch. https://github.com/gja/react-adaptive-hooks-ssr

Apologies for just copying the relevant file (react-adaptive-hooks/network), but I didn't have time to configure babel and whatnot for server side rendering.

Usage to start repo:

npm install
# npx webpack (if you want to recompile main.js, though it's checked in)
node .

As you can see from https://github.com/gja/react-adaptive-hooks-ssr/blob/master/src/ReactAdaptiveHooksExample.js, it should render a paragraph whose text and class are matching (ie: <p class="4g">Your Network Status Is 4g</p>). However, the ssr hydration will not resolve the classname (ie: <p class="unsupported">Your Network Status Is 4g</p>).

However, react hydrate does not go through element attributes to look for mismatches, hence these attributes will never be caught. (worse, react believes that the classname is set to 4g, while the dom has a different value, so this will not be rectified in future renders as well).

I'm not sure what the supported model is here. In the old class world, i'd have only called out to the new functions during componentDidMount, and change the behaviour that way. I'm not sure what the equivalent is in the react hooks world.

Maybe return undefined when loading, then useEffect to set loading to false, and return the correct value on the next render.

gja commented 4 years ago

I don't mind sending a PR as well to fix the issue, but I was wondering what direction you'd want to take (ie, is a 2 phase render type thing ok?)

wmertens commented 4 years ago

However, react hydrate does not go through element attributes to look for mismatches, hence these attributes will never be caught

😮 are you sure? This sounds like a React bug

wmertens commented 4 years ago

oh wow TIL

There are no guarantees that attribute differences will be patched up in case of mismatches -- https://reactjs.org/docs/react-dom.html#hydrate

Ok, so I propose a function you can call, setSSR(settings), which sets a module variable so results are what you define them to be, and then during hydration you use the same settings, and afterwards you call setSSR(false).

This is basically what I do, but I use Redux for it so it updates the tree where needed. So it's not a given that this should be solved in this module…

addyosmani commented 4 years ago

There are no guarantees that attribute differences will be patched up in case of mismatches -- https://reactjs.org/docs/react-dom.html#hydrate

Thank you for discovering this issue, @gja and for talking through one path forward @wmertens.

Ok, so I propose a function you can call, setSSR(settings), which sets a module variable so results are what you define them to be, and then during hydration you use the same settings, and afterwards you call setSSR(false).

My take on setSSR(settings) is that this may be a pattern we document, but I similarly agree I'm not sure it's within direct scope of this package to include it. I'm certainly not opposed to reconsidering.

Maybe return undefined when loading, then useEffect to set loading to false, and return the correct value on the next render.

What would be the trade-offs to this approach? Initial render would have to display undefined/some loading placeholder and we update to use the correct values and load media correctly on the next render?

gja commented 4 years ago

Yes, the problem is that it is an unnecesary render (and remember that you may generate an entire tree that you remove). This is inevitable if you are using SSR (since your Server Content may be coming from a CDN anyway), but a waste if you are a client only app.

My proposed API would look something like this:

const { effectiveConnectionType } = useNetworkStatus({ssr: 'loading'});

// How this would work would be something like this
function useNetworkStatusWithSSR({ssr = false}) {
  const [loading, setLoading] = useState(true); // <- You can't avoid calling this useState / useEffect here, because of the way react hooks works
  useEffect(() => setLoading(false));
  if(ssr && loading) {
    return { effectiveConnectionType: 'loading' }
  } else {
    return useNetworkStatus();
  } 
}
developit commented 4 years ago

Another option here would be to export a context provider from the module that enables/disables a "hydration mode". It could switch hooks to provide initial unknown values, then use useEffect or a setState callback to determine when hydration has completed and re-run any hooks that need client-side-specific output.

import { AdaptiveProvider } from 'react-adaptive-hooks';

hydrate(
  <AdaptiveProvider hydrate={true}>
    <App />
  </AdaptiveProvider>
);

My guess is that emulating the server's "unknown" state would be relatively straightforward since that codepath already exists for SSR.

h/t @devknoll for the context suggestion, which is much better than my initial idea of a singleton module with a global isHydrating flag.

mlampedx commented 4 years ago

On a related note, I think it would be beneficial to enable the developer to pass a custom value for the initialNetworkStatus.

A number of CDNs and other services are able to analyze the user’s connection speed and surface it to the web application server, which the developer can utilize. Of course, there’s no guarantee that this value will always match up with the value derived from the navigator on the client. In instances where the server-side value is reliable and consistent with the navigator value, populating the useNetworkStatus hook on the server can enable adaptive loading on SSR, reduce re-renders, and ensure HTML is consistent between SSR and client-side hydration.

addyosmani commented 4 years ago

On a related note, I think it would be beneficial to enable the developer to pass a custom value for the initialNetworkStatus.

@mlampedx

One benefit of this approach is that it could enable a developer to use Client Hints on the server to detect the effective network connection type then pass this value in to the hook as an initial value (vs opting for undefined).

I think this is an interesting idea to consider. I would be open to reviewing a PR adding support for this. We may want to consider that Client Hints can be used to expose a range of signals (incl. Device Memory and Save-Data) so this may be something to incorporate for things other than just network.

addyosmani commented 4 years ago

Another option here would be to export a context provider from the module that enables/disables a "hydration mode". It could switch hooks to provide initial unknown values, then use useEffect or a setState callback to determine when hydration has completed and re-run any hooks that need client-side-specific output.

I hadn't considered the idea of exporting a context provider that could facilitate a hydration mode. This is a very interesting suggestion, @developit. Could you expand on what you mean by "switch hooks to provide initial unknown values"? i.e are you suggesting we implement something similar to @gja / @mlampedx's proposals of controlling initial defaults if AdaptiveProvider is set to hydrate={true}? I understood the later part of your idea to re-run hooks requiring client-side output, but just wanted to make sure I was on the same page for this one part.

mlampedx commented 4 years ago

@addyosmani

Thank you for sharing the client hint documentation -- that is definitely in the vein of what I had in mind. Please find my implementation PR here.

gja commented 4 years ago

@mlampedx does your PR solve the issue of hydration having to match the SSR value? I still believe you'll have to do the useEffect / setState hack I mentioned in the beginning of the thread.

In such a case, I feel like it's better to pass an explicit value because there is a small impact of render being called twice.

mlampedx commented 4 years ago

@gja

It does not fix the mismatch issue, which is due to API asymmetry between the client and server, automatically. However, by using client hints to derive a reliable initial value to populate the hooks with on the server, the developer can ensure a consistent hook state and avoid this mismatch.

In cases where this option is unavailable to the developer, I think that we should either:

useEffect to calculate whether the prerequisite APIs are available now that the components are rendering on the client. This should avoid triggering a re-render in the event that the relevant APIs are unsupported in the user's browser. If the APIs are available, then I agree with you that a re-render is necessary.

E.g.

const [unsupported, setUnsupported] = useState(true);
useEffect(() => {
  const supported = 'connection' in navigator && 'effectiveType' in navigator.connection;
  setUnsupported(!supported);
});

or

Use the AdaptiveProvider with a hydrate flag -- a pattern that I have seen work well for SSR before. The provider's context could signal to the hooks when it is appropriate for them to re-run during the hydration phase.

wmertens commented 4 years ago

These are the cases to cover:

  1. Server-side rendering
  2. Client-side hydration
  3. Client-side first render without hydration
  4. Client-side subsequent renders

In 1, the values need to be guessed at so they are consistent with the client. In 2, the values need to be those used on the server, even if they are different from the client. In 3 and 4, the values should be the measured ones, either static or dynamic.

The AdaptiveProvider can set this up: it provides an AdaptiveContext that handles the SSR states, and if it's not used, the AdaptiveContext defaults are the measured values. By using a context, measurement happens only once, and all consumers are updated automatically.

So, the static values should be measured once and used as the default value for the AdaptiveContext. AdaptiveProvider gets guesses for the client values (SSR) or the server-guessed values (provided via some side channel), and uses those for the first render. On subsequent renders, it sets the context to the measured values.

For the SSR guesses side channel, one option would be to emit the guesses as data-x attributes which are read from the DOM on client load, but perhaps that is too invasive.