[react-hooks] initialize Media hooks with correct matches value

[melnikov-s]

Description

re-opening of https://github.com/Shopify/quilt/pull/2341

Description of original issue can be found there.

Forcing SSR support causes additional renders and potential layout shifts. To support CSR applications useMedia now sets the real value on initial render. For continued SSR support new options are provided to set the default value.

This is inline with similar hooks such as: https://usehooks-ts.com/react-hook/use-media-query

Fixes (issue #)

[melnikov-s]

/snapit

[melnikov-s]

/snapit

[jas7457]

I came across this when I saw there was a new major version. Won't this cause more issues than just simple SSR / hydration issues? Using window.matchMedia if initialValue isn't defined won't simply cause a hydration issue, it would throw on the server.

I'm not sure that changing something which, at worse before was a unnecessary render, to something which will start to throw by default, is a good idea.

@melnikov-s what do you think?

[melnikov-s]

@melnikov-s what do you think?

Yes, it will throw and initialValue now needs to be provided if you want to continue to support SSR thus the major version bump.

From my perspective throwing might even be preferable as it provides a more immediate feedback instead of a hydration mismatch which could be missed. Making the consumer aware that they now need to provide an initialValue

We could throw with a better error msg if initialValue === undefined && typeof window === 'undefined though

[jas7457]

If initialValue needs to be provided, it shouldn't be marked as an optional property. We should get TypeScript errors, not runtime errors, as depending on the way or order in which a route is rendered would determine if you'd see this error in the runtime or not.

You mentioned you modeled this off of other popular hooks like useMediaQuery, but you can see that they provide defaults for their values and they check if they're on the server or not.

I think we've enabled too many footguns to say it is both optional and there is no check if that we're not on the server for something like this.

[melnikov-s]

If initialValue needs to be provided, it shouldn't be marked as an optional property.

It only needs to be provided for SSR, for CSR it does not. Typescript has no awareness of this so can't really enforce that there.

I don't think there's a prefect approach here, they all have their cons. Though if you're doing SSR you want to have a consistent value during hydration and throwing an error can be seen as a forcing function of that.

That being said, I don't have a firm opinion on this. We can add the IS_SERVER check if there's consensus that it's better to have a hydration mismatch then to throw when we can't enforce this with types.

[vividviolet]

I would prefer to keep things simple if possible as this library is only being used by Web which no longer does SSR. Updating to initialValue === undefined && typeof window === 'undefined makes sense to me. typeof window === 'undefined is essentially the IS_SERVER check.

I think it's fine to make a breaking change with a breaking change log calling out that the initialValue is required if you are using it with server side rendering.

@jas7457 the example you showed has default the value to false which doesn't work for client side rendering. If we make the initial value mandatory then the consumer would have to do window. matchMedia to set the initial value.

[jas7457]

I think it's fine to make a breaking change with a breaking change log calling out that the initialValue is required if you are using it with server side rendering.

I would typically agree, that's what major changes are for, after all. However, the item in the change log simply says "#2809 5546b1d Thanks @​melnikov-s! - Media hooks initialized with correct matches value" - I don't see any mention about needing to make changes if you use SSR.

Remix is our blessed approach to Shopify apps now, which is a SSR library. I think that our default for this hook should be that it works for our blessed approach.

Updating to initialValue === undefined && typeof window === 'undefined makes sense to me.

Yes, this is really all we need, but I think you mean !== instead of ===