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

fix(useMediaQuery): Return boolean describing media query match on first render #1020

Merged
merged 5 commits into from
Dec 2, 2022

Conversation

ArttuOll
Copy link
Contributor

@ArttuOll ArttuOll commented Nov 20, 2022

What is the current behavior, and the steps to reproduce the issue?

Described in #1000 .

What is the expected behavior?

The value returned from the hook should be a boolean describing the media query match on first render instead of undefined.

How does this PR fix the problem?

By initializing the state with the the boolean received from matchMedia and creating a MediaQueryList entry to the cache in the process.

Here is a CodeSandbox, which contains the example provided in #1000, but with the new implementation of useMediaQuery. Note that the animation is no longer triggered when the page loads.

This is a breaking change, since the value returned on the first render is now different than before. (Returning undefined before was not a bug since it was documented)

Checklist

  • Have you read contribution guideline?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR?
  • Have the tests been updated to match the changes in the PR?
  • Have you run the tests locally to confirm they pass?

…rst render instead of undefined

This gets rid of the value changing from undefined to boolean on the first renders, which might
trigger undesired side-effects for users.

BREAKING CHANGE: On first render, boolean is returned instead of undefined.
fix #1000
@ArttuOll ArttuOll requested a review from a team November 20, 2022 15:46
@codecov
Copy link

codecov bot commented Nov 20, 2022

Codecov Report

Merging #1020 (f632445) into master (355e569) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1020   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          61       61           
  Lines        1041     1049    +8     
  Branches      163      166    +3     
=======================================
+ Hits         1037     1045    +8     
  Misses          2        2           
  Partials        2        2           
Impacted Files Coverage Δ
src/useMediaQuery/useMediaQuery.ts 100.00% <100.00%> (ø)
src/useScreenOrientation/useScreenOrientation.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xobotyi
Copy link
Contributor

xobotyi commented Nov 20, 2022

One problem - it will cause hydration error for SSR.

Initialisation should be flagged via hook parameters.

@ArttuOll
Copy link
Contributor Author

One problem - it will cause hydration error for SSR.

Initialisation should be flagged via hook parameters.

Sounds doable. I'll implement this later.

@ArttuOll
Copy link
Contributor Author

Btw, could you elaborate on how this causes an hydration error? I wasn't aware of this.

@xobotyi
Copy link
Contributor

xobotyi commented Nov 20, 2022

SSR will be first-rendered with undefined, client with some value - react will freak out on that.
Basically to make SSR work - first render on server and in browser should match, meaning that components states and resulted DOM trees should match exactly.

@xobotyi
Copy link
Contributor

xobotyi commented Nov 20, 2022

Also I have an idea to replace all that crap with initializeWithValue props in favor of context provider that will automatically tell all our hooks that we're in SSR mode without the need of passing extra arguments to hooks

@ArttuOll
Copy link
Contributor Author

first render on server and in browser should match

Of course, makes sense! Thanks.

Also I have an idea to replace all that crap with initializeWithValue props in favor of context provider that will automatically tell all our hooks that we're in SSR mode without the need of passing extra arguments to hooks

Interesting. Would the users then have to wrap their app in some sort of context provider?

@xobotyi
Copy link
Contributor

xobotyi commented Nov 21, 2022

Yes, they should, but in my eyes it is way more convenient that to be sure that you pass proper hook config to each hook that has isomorphic behavior.

This allows us to give the users the media query match state immediately on first render and
optionally give them SSR compatibility.

BREAKING CHANGE: Returned value is no longer undefined on first render. Set initializeWithValue to
false to re-enable this behavior.
fix #1000
@ArttuOll
Copy link
Contributor Author

@xobotyi How about something like I pushed in my last commit? I added initializeWithValue option and defaulted it to true, like with other hooks such as useLocalStorageValue etc.

… optionally with undefined

This makes the hook work well with its underlying useMediaQuery hook. Also, many other hooks follow
this convention of initializing with correct value by default and optionally with undefined.

BREAKING CHANGE: Previously this hook initialized with undefined. Now it initializes by default with
the correct hook value. The previous behavior can be enabled by setting the initializeWithValue
option to false.
@ArttuOll ArttuOll requested a review from xobotyi December 2, 2022 06:45
@xobotyi xobotyi merged commit 087b2b1 into master Dec 2, 2022
@xobotyi xobotyi deleted the pr/fix-1000 branch December 2, 2022 17:11
github-actions bot pushed a commit that referenced this pull request Dec 2, 2022
# [20.0.0](v19.2.0...v20.0.0) (2022-12-02)

### Bug Fixes

* **useMediaQuery:** add option to martch media query on first render ([#1020](#1020)) ([087b2b1](087b2b1)), closes [#1000](#1000)

### BREAKING CHANGES

* **useMediaQuery:** `useMediaQuery` and `useScreenOrientation` now returns matched media query state
on first render by default, SSR users can change that behaviour via hook options.
@xobotyi
Copy link
Contributor

xobotyi commented Dec 2, 2022

🎉 This PR is included in version 20.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants