Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from

Conversation

melnikov-s
Copy link
Contributor

Description

This PR addresses an issue with the media hooks where the initial render will always return the value of false regardless if the media matches or not, only on subsequent renders will the correct value be set. This causes a needless re-render if the media matches as well as could cause issues when using the results in a useEffect (for example when using tracking events but only when media matches)

Note: I've set this is a breaking change that requires a major version bump as it technically a change in behaviour and it's possible that there is code relying on the old behaviour.

@melnikov-s melnikov-s requested a review from cameronbaney July 4, 2022 16:19
@melnikov-s melnikov-s requested a review from a team as a code owner July 4, 2022 16:19
@melnikov-s melnikov-s requested a review from CameronGorrie July 4, 2022 16:19
@@ -66,6 +70,7 @@ describe('useMedia and useMediaLayout', () => {
);

const mockComponent = mount(<MockComponent mediaQuery="print" />);
expect(matches).toStrictEqual([true]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use of act within the mount hides the initial value of false here, needed an alternative way to check what's actually being set during each render. Without this change matches is [false, true]

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test these changes with SSR.

I suspect this will cause hydration mismatch errors as what gets returned from the server is "matches nothing" but the initial render shall match some media query, and thus React will throw warnings saying "The first render did not match what was returned from the server".

@melnikov-s
Copy link
Contributor Author

Please test these changes with SSR.

Great catch! Yeah there will be issues with SSR though this does seem like the approach taken by most of these useMedia type hooks:

https://github.com/chakra-ui/chakra-ui/blob/main/packages/media-query/src/use-media-query.ts
https://usehooks-ts.com/react-hook/use-media-query
https://github.com/mui/material-ui/blob/master/packages/mui-material/src/useMediaQuery/useMediaQuery.ts

They all provide a fallback that you have to manually pass in for SSR.

Despite the drawbacks, the current approach does work for SSR and given the heavy usage across admin and beyond it's probably not worth proceeding with this PR.

Though I do wonder, if it makes more sense to return undefined on first render instead of false as the question of whether a device matches media during SSR is non deterministic and therefore undefined seems like the appropriate return type for that.

Will help in this kind of situation

const isLargeScreen = useMedia('(min-width: 768px)');
const [monorailViewed, setMonorailViewed] = useState(false);
useEffect(() => {
  if (!monorailViewed || isLargeScreen === undefined) return;
  if (isLargeScreen) {
    trackLargeScreenMonorail();
  } else {
    trackSmallScreenMonorail();
  }
  setMonorailViewed(true)
}, [
  }, [isLargeScreen, monorailInboxAdminOverviewAnalyticsViewed, monorailViewed]);

Though the above is also possible with what's there today by using useMedia twice once for isLargeScreen and again for isSmallScreen with an inverse query.

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

Successfully merging this pull request may close these issues.

3 participants