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 Adaptive Hooks will cause attribute mismatch during server side rendering #18

Open
gja opened this issue Nov 14, 2019 · 13 comments
Open

Comments

@gja
Copy link

gja commented Nov 14, 2019

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
Copy link
Author

gja commented Nov 14, 2019

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?)

@gja gja mentioned this issue Nov 14, 2019
@wmertens
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Author

gja commented Nov 14, 2019

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
Copy link

developit commented Nov 15, 2019

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link
Author

gja commented Nov 18, 2019

@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
Copy link
Contributor

@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
Copy link
Contributor

wmertens commented Nov 21, 2019 via email

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

No branches or pull requests

5 participants