[useMediaQuery] Fix hydrationCompleted true before hydrated #18683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix a bug where the `hydrationCompleted` variable in `useMediaQuery()` can be set to `true` before all components in the document are actually hydrated. Apply the patch suggested in mui#18670 that removes this optimization in favor of more consistent rendering behavior. Modify the test for this optimization to check for its absence, and elsewhere, remove the use of the `testReset()` test helper function.
2 tasks
oliviertassinari
changed the title
oliviertassinari
approved these changes
Dec 4, 2019
Details of bundle changes.Comparing: dfe2779...afdffe5
|
|
@toddmazierski It's a great first pull request on Material-UI 👌🏻. Thank you for working on it! |
2 tasks
2 tasks
theKashey
reviewed
May 22, 2020
| @@ -54,7 +51,6 @@ function useMediaQuery(queryInput, options = {}) { | |||
|
|
|||
| React.useEffect(() => { | |||
| let active = true; | |||
| hydrationCompleted = true; | |||
There was a problem hiding this comment.
So the depth-first use effect would "complete" hydration for everyone.
This flag could be set only on top level component as long as its useEffect would be called after all children ones.
There was a problem hiding this comment.
@theKashey Interesting, a top-level use effect could be a good edge in case progressive hydration https://youtu.be/k-A2VfuUROg?t=993 come officially to React 😁.
There was a problem hiding this comment.
I am using the hack from the video for last two years (well used a bit less performant hack before)
There was a problem hiding this comment.
I start to think that we need a second top-level Provider for Material-UI (in addition to ThemeProvider), maybe a ConfigProvider that bundles different concerns so we don't end up with this API, but we can still use different context internally:
How
Many
Providers
Until
We
Say
It's
Enough?
A provider that could host:
- Number formatting [l10n] Prepare iteration on number formatting #20656 (comment)
- Date formatting https://next.material-ui-pickers.dev/getting-started/installation
- Date parsing https://next.material-ui-pickers.dev/getting-started/installation
- Snackbar/Dialog imperative API https://github.com/iamhosseindhv/notistack#how-to-use, https://trello.com/c/iNtH8wgp/2176-dialog-imperative-api.
- Client-side rendering only [useMediaQuery] always returns false at the first call #21142 (comment)
- … maybe more in the future
cc @mui-org/core
There was a problem hiding this comment.
I am using the hack from the video for last two years (well used a bit less performant hack before)
Oh, so the useEffect at the root is a dead end? Each partially hydrated tree would need its own provider. I think that we can stick to the noSsr boolean, simpler.
There was a problem hiding this comment.
Oh, so the useEffect at the root is a dead end?
No-no, it works just perfectly. And the additional Provider to update SSR status would be just great.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a bug where the
hydrationCompletedvariable inuseMediaQuery()can be set to
truebefore all components in the document are actuallyhydrated.
Apply the patch suggested in #18670 that removes this optimization in
favor of more consistent rendering behavior.
Modify the test for this optimization to check for its absence, and
elsewhere, remove the use of the
testReset()test helper function.Closes #18670