[useMediaQuery] Accept function as argument & more

[merceyz]

• I have followed (at least) the PR section of the contributing guide.

Closes #16313
Closes #15187
Closes #16073
Closes #16580

[merceyz]

@oliviertassinari Shouldn't useMediaQueryTheme be in it's own folder next to useMediaQuery?

[mui-pr-bot]

Details of bundle changes.

Comparing: 783b693...2d84f2d

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.07%	-0.01%	327,763	327,524	90,314	90,301
@material-ui/core/Paper	0.00%	0.00%	68,477	68,477	20,407	20,407
@material-ui/core/Paper.esm	0.00%	+0.02% 🔺	61,761	61,761	19,190	19,194
@material-ui/core/Popper	0.00%	+0.01% 🔺	28,942	28,942	10,407	10,408
@material-ui/core/Textarea	0.00%	+0.04% 🔺	5,505	5,505	2,365	2,366
@material-ui/core/TrapFocus	0.00%	0.00%	3,753	3,753	1,576	1,576
@material-ui/core/styles/createMuiTheme	+0.01% 🔺	0.00%	16,160	16,161	5,813	5,813
@material-ui/core/useMediaQuery	+19.38% 🔺	+18.93% 🔺	2,595	3,098	1,104	1,313
@material-ui/lab	0.00%	-0.04%	138,027	138,027	42,562	42,547
@material-ui/styles	0.00%	-0.03%	51,887	51,887	15,384	15,380
@material-ui/system	0.00%	+0.07% 🔺	15,576	15,576	4,432	4,435
Button	0.00%	+0.01% 🔺	81,161	81,161	24,779	24,781
Modal	0.00%	-0.08%	14,551	14,551	5,102	5,098
Portal	0.00%	0.00%	3,471	3,471	1,573	1,573
Slider	0.00%	-0.03%	75,100	75,100	23,309	23,302
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	54,338	54,338	13,762	13,762
docs.main	-0.04%	-0.00%	647,863	647,633	204,220	204,215
packages/material-ui/build/umd/material-ui.production.min.js	-0.10%	-0.03%	300,178	299,880	86,044	86,014

Generated by 🚫 dangerJS against 2d84f2d

[joshwooding]

@oliviertassinari Shouldn't useMediaQueryTheme be in it's own folder next to useMediaQuery?

IMO it's probably fine as it is now. I guess it could be placed in its own folder but it's just useMediaQuery with the default theme injected not sure if it differs enough to need to be in it's own folder.

Plus moving it might be a breaking change depending on imports.

[oliviertassinari]

I have to admit, I shouldn't have added useMediaQueryTheme in the documentation. It was rushed.
The module isn't exported anywhere, nor present in any demo. People can find it searching in the source. We also have one reference of its existence in the documentation. People importing it break the tree-sharking rule (depth > 2), they most likely bundle the cjs and esm transitive dependencies of the module. That's not great.

It would be great to get it right this time! One possible strategy:

• We could merge the current useMediaQueryTheme.js logic into useMediaQuery.js. I wouldn't expect any significant bundle size increase. The theme has no default value. Regarding the naked version of the library, we would expose a hook factory where people can provide their own theme context.
• Do we need to expose a version of the hook with the default theme cc @fzaninotto? If we do (most likely), then maybe we should have a separate new public useMediaQueryTheme module.

[fzaninotto]

I totally agree to merge useMediaQuery and useMediaQueryTheme into one hook with a switch. It would simplify documentation and usage.

If the theme has no default value, there needs to be an escape hatch for the function version, because if a developer calls useMediaQuery(theme => theme.breakpoints.up('sm')) and theme is undefined, the function will break. So if a function is passed and useTheme doesn't return anything (e.g. in unit tests), then the function shouldn't be called and the hook should return false.

[oliviertassinari]

@fzaninotto I don't think that useMediaQuery should have a default theme for bundle size reason. The theme object weight 2kB gzipped, it would more than double the weight of the hook. However, useMediaQueryTheme could :).

[merceyz]

Plus moving it might be a breaking change depending on imports.

@joshwooding Based on its location it's considered a private component, but technically yes, it would be.

[oliviertassinari]

Before we move forward in any direction. I would propose that we take a step back by taking a holistic point of view. We could potentially improve this hook in the following areas:

1. JavaScript media query support. So far, we only accept a string media query

useMediaQuery('(min-width:600px)')

We could support an object syntax:

useMediaQuery({ minWidth: 1000 });

The bundle overhead seems minimal: https://github.com/streamich/use-media/blob/3093c39a2ac119738f887c43d503ddfa77f8c6d3/src/index.ts#L8-L25. Should we support it outside of the box rather than documenting a usage example with json2mq?

2. Multi-media query support. It's something the media query support but is private. It was introduced when updating the withWidth.js higher-order logic: [withWidth] Migrate to hooks #15678. Without it, people can experience glitches in the intermediary state: useMediaQuery return wrong width #16460, as well as, two renders when only one is necessary. Should we make this API public? Should we remove it? Should we keep the current status-quo?

3. Theme callback. It's described in [useMediaQueryTheme] Accept function as first argument #16313. It removes the need to import and use useTheme. Should we support it? Should we provide the default theme?

4. Server-side rendering support. As explored in Fix useMediaQuery server-side example #16312, this is a challenging one. We have to rely on the context for propagating an estimation of the client configuration: optimization: use client hints for SSR yocontra/react-responsive#204.

Based on its location useMediaQueryTheme is considered a private component.

@merceyz Yes, I agree. But we mention its existence at one place in the docs.

What do you guys think of:

• We add support for the object syntax.
• We keep the array support private (hence closing useMediaQuery return wrong width #16460, cc @siriwatknp), so maybe we can remove it in the future.
• We merge useMediaQuery.js with useMediaQueryTheme.js (so we remove the private module)
• Adding support for the theme callback.
• If/When we move [core] POC around @material-ui/naked #16238 forward, we can have two modules @material-ui/unstyled/useMediaQuery and @material-ui/core/useMediaQuery.
  The difference between core and unstyled would be that core has the default theme, unstyled don't.

[eps1lon]

Multi-media query support

What's that? CSS media queries already support chaining multiple queries.

JavaScript media query support

What would be the advantages here? Why do we need it in the core?

Theme callback

It's one tiny import. I think we that we can live with it.

Server-side rendering support

I think this shouldn't be a hook that's used on the server. Media queries make no sense on the server. It should rather be implemented in application code where a default bundle is shipped and then adjusted on the client. People need to be aware that changing layout of a server-side rendered app with client side logic will cause flash of unstyled content (or rather "wrongly styled content").

[oliviertassinari]

What's that? CSS media queries already support chaining multiple queries.

Yes, media queries support logic. The multi-media query feature is only interesting for batching the updates. #16460 explore the interest of this feature.

Assuming you need to do:
A.

const a = useMediaQuery('(min-width: 100px)');
const b = useMediaQuery('(max-width: 150px)');

what if you could write instead:
B.

const [a, b] = useMediaQuery(['(min-width: 100px)', '(max-width: 150px)']);

and only re-render once when a media query match changes? I believe that the update batching only works because React deduplicates the state updates of the same useState hook (B.). It doesn't work when the state is in two different useState hooks (A.).

Now, the only close to an "acceptable" use case we have for it is the withWidth. It's why I'm reluctant to make it public and hopefully remove it. None of the alternative implementations I can benchmark support it:

• https://github.com/streamich/use-media
• https://github.com/ReactTraining/react-media
• https://github.com/contra/react-responsive
• https://github.com/thearnica/react-media-match

What would be the advantages here? Why do we need it in the core?

It's a feature request I have seen from people, I wish I have kept the source of it! I can imagine that it can be more convenient than using a string template with variables. I don't have a strong conviction that it's very valuable. https://github.com/streamich/use-media supports it. Maybe we should wait for further requests with a strong case?

Media queries make no sense on the server.

It's fundamentally impossible to perform media queries server side, correct. The server has some information on what the client will look like when rendering its response but not enough. People can use heuristics on the user-agent or leverage client hints. It's these cases that I think we should try to better support.

• https://github.com/ReactTraining/react-media#server-side-rendering-ssr
• https://github.com/thearnica/react-media-match#server-side-rendering
• https://github.com/contra/react-responsive#server-side-rendering

How can we help them minimize the wrongly styled content (avoiding wrong intermediate state)?

[eps1lon]

and only re-render once when a media query match changes?

Needs benchmark showing that one is faster than the other. A render call is nothing inherently slow. Quite the opposite. Don't get me wrong it's not like this is very complicated and adds a lot of maintenance burden. I just want to avoid further propagating that you should care about the number of render calls. You really don't and if they are an issue than you should look into your render call. Are media queries even that time sensitive? Do I really need to have 60fps while resizing my window?

How can we help them minimize the wrongly styled content?

I don't think we can. There's just an inherent disconnect between the static markup being rendered and javascript being evaluated/executed

[oliviertassinari]

I just want to avoid further propagating that you should care about the number of render calls

In #16460, the issue is with the consistency of the data.
A second one can be about the noise it creates when debugging.
For performance, I agree that it's not systematic, it depends on what you render, sometimes, it can be perfectly fine. I'm wondering even if it's not in the majority of the cases that it's fine. Then, I think that what's important is the perceived value from the users, it takes time to diffuse people fears.

I don't think we can.

Assuming that 70% of the media query usage is about width responsiveness and that 70% of the browsers support width client hint. What prevents us from helping people to solve the issue?
However, ~15% of people do SSR (dynamic or static). I would expect very few to try to solve this problem. It's probably not very important.

[merceyz]

what if you could write instead:
const [a, b] = useMediaQuery(['(min-width: 100px)', '(max-width: 150px)']);

In that case I feel using objects would be easier to read/use + autocomplete
const matches = useMediaQuery({ minWidth: 100, maxWidth: 150 });

[eps1lon]

A third one can be about the noise it creates when debugging.

There are plenty of tooling available to filter noise. If you want to listen for it then that is your issue. Again: a render is not important. A commit might be but even then: Fix your tooling and filter the noise of commits below 16ms.

In #16460, it's about consistency of the data.

Makes sense. Though I sense there is a different issue here with how the intervals are constructed. We have to be aware that we're kind of abusing media queries to read the current screen width. They were not really intended to do that.

[oliviertassinari]

Regarding the logging aspect, my assumption was that the majority of the people use console.log and that it's not likely to change.

[oliviertassinari]

remove multiple support

[merceyz]

@oliviertassinari Wouldn't it be better if @material-ui/styles had the version of this hook without the default theme while @material-ui/core has one with it?

[oliviertassinari]

@merceyz Yes, I think that we could have two versions in the future:

• @material-ui/unstyled without the default theme
• @material-ui/core with the default theme

Something for the coming months :), for now, I think that core without the default theme is our best option.
@material-ui/styles should focus on the styles, we could even eventually kill it for a more popular one. I don't think that it should contain any hook/component.