Two-pass rendering is broken in the hooks implementation

[tstirrat15]

See comment here: https://github.com/ReactTraining/react-media/issues/110#issuecomment-647536404

In my head this points to a need for a test of two-pass rendering behavior, comparing the states of:

1. server-side render
2. first client-side pass (i.e. what would happen during hydration, before didMount/useEffect runs)
3. second client-side pass

I'm pretty sure this is the set of expectations:

• if defaultMatches is set, the first two would match and the third would not
• if defaultMatches is not set, the first clientside pass should be complete
• if defaultMatches is set and we're only on the client, we should get two renders.

Please correct me if the above is incorrect.

[tstirrat15]

Other references:

• https://joshwcomeau.com/react/the-perils-of-rehydration/
• Previous issue: https://github.com/ReactTraining/react-media/issues/81
• Previous PR: #96

[tstirrat15]

This could be a good time to get hook testing in place, too: https://react-hooks-testing-library.com/reference/api

[edorivai]

I'm pretty sure this is the set of expectations:

• if defaultMatches is set, the first two would match and the third would not
• if defaultMatches is not set, the first clientside pass should be complete
• if defaultMatches is set and we're only on the client, we should get two renders.

Please correct me if the above is incorrect.

Minor tweak here: these expectations assume that defaultMatches are not equal to the actual matches as determined by the media query.

[gpoole]

I think the requirement is to force a second pass unless the client-side query result exactly matches defaultMatches, even in the default case. The first pass in the two-pass render needs to match exactly what the server rendered, then the second pass can apply client-side changes.

I'd say for this hook if the query doesn't match defaultMatches on the client side, the hook should handle the two pass case by always returning defaultMatches first (so the first render after mounting matches exactly what SSR did) then update and return the actual matches after mounting (second render is the one that updates the client to match queries).

Sketching it out, I think changing useMedia along these lines would work as I described:

   // 1st render pass returns default matches or default all-true match
    const [matches, setMatches] = useState(() => {
      // clientOnly option could force the 2nd pass off if it's safe to do so or we could invert this and require a flag to enable 2 pass behaviour
      if (clientOnly) {
        setUpMQLs();
        return getMatches();
      }
      if (defaultMatches) {
        return defaultMatches;
      }
       return Object.keys(queries).reduce(
          (acc, key) => ({ ...acc, [key]: true }),
          {}
        );
    });

  useEffect(
    () => {
      // If clientOnly flag is set we don't need to update anything
      if (!clientOnly) {
        // Moved from useState callback
        setUpMQLs();
        // Force check and 2nd pass render with the actual media queries if anything changed
        const clientMatches = getMatches();
        // Check if the matches in the client have changed from the default value
        // (Needs a shallowEqual implementation from somewhere :()
        if (!shallowEqual(clientMatches, matches)) {
          setMatches(clientMatches);
        }
      }
      return () => activeQueries.current.forEach(({ mqListener }) => mqListener.cancel());
    },
    [],
  );

 // snip

  return matches;

Not sure if that brings in any other issues. What do you think?

Edit: Noticed a bug with the clientOnly flag causing the queries to get stuck on default values after I posted, updated the code to fix that hopefully.

[tstirrat15]

I'll implement it and see if this fixes it. Thank you!

[jonmajorc]

Any update on this one? 😃

[mrmartineau]

@jonmajorc FYI https://blog.sindresorhus.com/issue-bumping-e3b9740e2a0

[jonmajorc]

@mrmartineau The blog did say it's okay to bump when...

it looks like there was supposed to be some activity, but there weren’t.

So I just wanted to see... 🤷‍♂️

[edorivai]

Hey @jonmajorc, unfortunately no updates from my end. As pointed out in the sindresorhus article, I currently don't have the bandwidth to actively take up issues here. I'm trying to keep an eye on the repo and I'll try to help move PRs forward.

[jonmajorc]

@edorivai no worries!! Just seeing! I too don't have the bandwidth as of now, so I totally understand 😄 -- Thanks for the upate!