SSR; statics

[wmertens]

I noticed that none of the x in navigator statements are guarded, so they will all break when rendering server-side. I propose adding typeof navigator !== 'undefined' in front of all those.

Furthermore, with the exception of the networking, the hooks return static data and even use useState to store the static data, causing extra work and memory use for something that's basically a single static value that can be determined at module load time. This confuses me, I created #1 to explain what I mean.

[addyosmani]

Thanks for the suggestions!

I noticed that none of the x in navigator statements are guarded, so they will all break when rendering server-side. I propose adding typeof navigator !== 'undefined' in front of all those.

I'm supportive of us introducing a guard to avoid breakage during server-side rendering. This is a good call and is something we discussed but didn't circle back on. I'd be open to a PR with this change.

Furthermore, with the exception of the networking, the hooks return static data and even use useState to store the static data

Shifting to a model where we treat all hooks short of network as providing static values likely makes sense. We started off our implementation looking at additional APIs that fired change events, but given the current scoped set doesn't heavily do this, a change here sgtm as long as tests pass.

[wmertens]

@addyosmani so your preferred API would still require calling an ersatz hook that returns the static values, in case these values become dynamic in the future?

I think the tests can pass by messing with require.cache, I'll take a look.

[wmertens]

ok, the static hooks keep their API and the tests pass in #1

[addyosmani]

Thanks once again for #1, @wmertens. I believe with the guards in place and shift away from using useState we should be good on the SSR side here. Just wanted to check there isn't anything remaining to be done for this issue before I close it?

[wmertens]

No, I can't think of anything else 👍

[gja]

@wmertens @addyosmani Wouldn't this cause hydration to fail? Something like useNetworkStatus would return different results server side and client side, and I'm not sure if react would handle it correctly in all cases.

Usually what is done is to move rendering to three phases

• Once Server Side
• Once client side during hydration, where the value is same as the server side
• Update a flag on componentDidMount / useEffect, which causes the value to be set correctly

[wmertens]

Right, but in this case you probably want hydration to fail, so that the conservative estimate that the server did is replaced with the correct estimate on the client side. React will only fail to hydrate the subtree, so it should work optimally.

[gja]

@wmertens I don't think react will fail. I have created a sample repository demonstrating the issue, and moved this into a new issue over here: https://github.com/GoogleChromeLabs/react-adaptive-hooks/issues/18