[withWidth] Migrate to hooks #15678

jacobbogers · 2019-05-13T13:21:53Z

Hi, before I do more work, I would to know if this is kindof ok
I havent updated the docs, although i did write some unit tests using your test utils

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

Closes #14101

jacobbogers · 2019-05-13T15:17:20Z

I am not sure why "argos" is failing the checks, can someone advise,
thanks

eps1lon · 2019-05-13T16:32:31Z

It would be nice if we could clear up first what problem you are trying to solve, how the API looks like etc.

jacobbogers · 2019-05-13T19:21:00Z

@eps1lon ,to answer your question see the following.
follow the discussion here with @Olivier-Tassinari #14101
cross linked with umbrella issues about links #15231
Umbrella issue (also mentioned in this PR)
There is a minimalist demo (mentioned in 14101)
The unit tests show usage also

useWidth (akin the useTheme ) standalone returns the width
WithProvider context Provider (handly if you want to wrap a lot of components who need to redraw themselves based on width, avoid creating multiple matchMedia query lists for every component that needs to know width)
useWidthContext ( plucks the width from the WidthProvider)
For the use of widthProvider+useWidthContext, example at https://github.com/jacobbogers/benchmark-mobile/blob/master/src/App.js

jacobbogers · 2019-05-13T19:31:30Z

Lets keep the PR only for technical integration issues, for the discussions about design lets keep it in the issue tickets otherwise it spreads over 3 places.

I apologize if the issue was not clearly linked in this PR

mui-pr-bot · 2019-05-13T19:56:22Z

Details of bundle changes.

Comparing: 4fccafb...0e59653

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.03%	-0.06%	317,449	317,365	86,962	86,910
@material-ui/core/Paper	0.00%	-0.00%	67,921	67,922	20,186	20,185
@material-ui/core/Paper.esm	0.00%	+0.06% 🔺	61,217	61,217	18,978	18,989
@material-ui/core/Popper	0.00%	-0.02%	28,728	28,728	10,345	10,343
@material-ui/core/Textarea	0.00%	-0.17%	5,513	5,513	2,377	2,373
@material-ui/core/TrapFocus	0.00%	0.00%	3,755	3,755	1,575	1,575
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	15,978	15,978	5,786	5,786
@material-ui/core/useMediaQuery	+20.09% 🔺	+11.72% 🔺	2,106	2,529	973	1,087
@material-ui/lab	0.00%	0.00%	138,321	138,322	42,486	42,488
@material-ui/styles	0.00%	-0.05%	51,386	51,386	15,194	15,186
@material-ui/system	0.00%	0.00%	14,463	14,463	4,178	4,178
Button	0.00%	-0.05%	83,902	83,904	25,467	25,454
Modal	+0.01% 🔺	-0.04%	20,342	20,344	6,688	6,685
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	55,371	55,371	13,914	13,914
docs.main	-0.71%	-0.57%	649,679	645,065	205,139	203,968
packages/material-ui/build/umd/material-ui.production.min.js	-0.09%	-0.28%	291,344	291,075	83,270	83,041

Generated by 🚫 dangerJS against 0e59653

eps1lon · 2019-05-14T09:38:53Z

Seems like this is based on an outdated version of the next branch. Otherwise I don't understand why we to change the dependency versions.

oliviertassinari · 2019-05-14T10:07:29Z

@jacobbogers Thanks for your time! However, did we agree on a final direction before starting the implementation? (aside from)

I believe the useWidth() can be implemented in about 10 lines of code: https://next.material-ui.com/components/use-media-query/#migrating-from-withwidth. Now, It's probably too hacky. If the theme is dynamically changed to contain a different number of breakpoints, we will break the hook call order rule. A better solution should probably be about 100 lines of code. 1000 is not the same order of magnitude, something is off:

Do we really need a useWidth hook method now that we have the useMediaQuery? The alternative popular solutions don't support it:

Maybe it should be published as a different package?

I understand why a useWidth hook is handy for symetry with withWidth. I don't understand why we need a provider for that in the core. Seems like this is better suited for app code.

I agree, I don't think that it's needed. I believe we have an existing mechanism for providing a default value when rendering on the server: theme.props.MuiWithWidth.defaultWidth = 'md'. Can we use it?

jacobbogers · 2019-05-14T18:00:59Z

@eps1lon Thank you, yes you are correct i see in yarn.lock the version of react-docgen to be 5.0.0-beta, 6 days ago, I was using version 3.0.0. I pulled yesteday from upstream branch (this repo i forked from) but something not quite correctly merged I think.

@oliviertassinari ,
The implementation is about 97 lines of code,)))

useWidth 63 lines of code
useWidthContext 6 lines of code
WidthProvider 28 lines of code

the rest of the code is for mock objects, typescript decl files, and for unit testing. Yes my implementation of useWidth, and WidthProvider does allow to define your own theme.

the mocks (only for testing purpose)

made mock for the screen (you can dynamicly change screensize and watch the hook "re-fire")
made mock for windows.matchMedia
made mock for matchMediaQueryList (all public properties and methods)
wrote a unit test using these mocks

For the motivation why i wrote a Provider.., I repost my answered to @eps1lon
#14101 (comment)

@eps1lon,
On the "Provider",..., I was reasoning along the lines of react not having dedicated native event handlers (mouseclick) for every component. So just measure (via mediaQueryList) the screensize in one place (the Provider) and pas it down to the many components who need this knowledge.

So it was more of a performance enhancer,

for a single component who needs to know width, just use the seperate useWidth directly (no Provider needed), hence I thought it would be a good idea to provide both, and have the dev choose depending on use case.
_

I can scrap the WidthProvider and useWidthContext if you think its too much, and use your useMediaQuery if you want, no problem.

oliviertassinari · 2019-05-15T21:35:17Z

The implementation is about 97 lines of code,)))

@jacobbogers Nice, it's the order of magnitude I was hoping for :). Yes, I think that we should the existing infrastructure instead of anticipating a problem (no custom provider).

oliviertassinari · 2019-05-26T21:10:29Z

@jacobbogers How is it going?

oliviertassinari · 2019-06-02T09:52:18Z

I'm continuing the effort.

packages/material-ui/package.json

oliviertassinari · 2019-06-02T14:04:20Z

docs/src/pages/components/use-media-query/use-media-query.md

+
+[json2mq](https://github.com/akiran/json2mq) is used to generate media query string from a JavaScript object.
+
+{{"demo": "pages/components/use-media-query/JavaScriptMedia.js", "defaultCodeOpen": true}}


We can come back to it in two months, we will be able to use the Google Analytics events to better answer this question: Should we have json2mq built-in?

jacobbogers · 2019-06-02T15:26:35Z

HI Toni, sorry for being afk, had a lot to do,
Is there anything else that needs on this ticket that needs work?

oliviertassinari · 2019-06-02T15:34:45Z

It would be great if you could review the changes :)

jacobbogers · 2019-06-02T16:41:59Z

Looks great @oliviertassinari , I als see how you test it all.

eps1lon · 2019-06-08T14:16:03Z

packages/material-ui/src/useMediaQuery/useMediaQuery.js

 function useMediaQuery(queryInput, options = {}) {
- const query = queryInput.replace('@media ', '');
+ const multiple = Array.isArray(queryInput);


Just seeing this since it was not obvious that any implementation changed from the PR title.

What is the use case for this? media queries already support combining multiple queries with boolean logic. This bloats the code a lot. It's nowhere documented that this doesn't support changing the number of queries provided.

What is the use case for this?

The use case is useWidth. I have first tried to implement it with the existing useMediaQuery.js logic. It wasn't possible. It was making the component render x2 x card(breakpoints), so around 10 renders.

media queries already support combining multiple queries with boolean logic.

And yet, it returns a single boolean.

This bloats the code a lot.

I have seen worse

doesn't support changing the number of queries provided.

It should be supported.

It was making the component render x2 x card(breakpoints), so around 10 renders.

Did you profile it?

And yet, it returns a single boolean

Can always use multiple hooks. No need to overload the code.

doesn't support changing the number of queries provided.

It should be supported.

It is passed as a dependency errors to hooks.

Did you profile it?

The withWith() higher-order component was making his wrapped component render 10 times. The performance impact is not the only dimension to consider. The more renders, the harder debugging experience for people using this higher order component.

This change wasn't released yet. It's not too late to revert it if you don't feel confident with the direction. You can play with the alternative here: https://material-ui.com/components/use-media-query/#migrating-from-withwidth. Maybe if we ignore the dynamic breakpoint occurrence, and that we use an intermediary pure component to remove the redundant renders, it would work better.

An interesting implementation to benchmark against: https://github.com/streamich/use-media/blob/master/src/index.ts.

Ok, I will give it a second try.

I can't find a way to deduplicate the renders.

Notice that I haven't updated the useMediaQuery TypeScript signature nor documented the array existance. What do you of this middle ground solution? We revert the useWidth migration example so we can, later, continue iterate on this topic without being lock-in with the array behind a public API, used by people.

henrylearn2rock · 2019-06-19T17:14:09Z

Since we can't use Hooks inside of a class component, can we not deprecate withWidth()?

quote from : https://material-ui.com/customization/breakpoints/#withwidth

⚠️ This higher-order component will be deprecated for the useMediaQuery hook when the React's hooks are released as stable.

React hook is already stable.
quote from : https://code.fb.com/open-source/react-hooks/

React 16.8 is the first stable React release with support for Hooks.

joshwooding · 2019-06-19T18:11:40Z

@henrylearn2rock We had a lot of feedback asking for a HOC to still be available #14101

oliviertassinari · 2019-06-20T10:53:17Z

@henrylearn2rock Thanks for raising the "once stable" issue. We should update the documentation.
Ideally, I wish we can kill this withWidth higher-order component in v5. It might not be possible, as many people still rely on it.

Don't rely on it for new code.

jacobbogers mentioned this pull request May 13, 2019

Migration to hooks #15231

Closed

29 tasks

eps1lon added new feature New feature or request package: material-ui Specific to @mui/material labels May 14, 2019

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 18, 2019

oliviertassinari changed the base branch from next to master May 23, 2019 21:08

This was referenced May 31, 2019

[core] Import esm babel helpers #15979

Closed

recompose dependency has been removed but the documentation still uses it #15954

Closed

oliviertassinari self-assigned this Jun 2, 2019

oliviertassinari changed the title ~~useWidth, useWidthContext, WidthProvider hooks/providers~~ [withWidth] Migrate to hooks Jun 2, 2019

oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jun 2, 2019

oliviertassinari reviewed Jun 2, 2019

View reviewed changes

packages/material-ui/package.json Outdated Show resolved Hide resolved

oliviertassinari reviewed Jun 2, 2019

View reviewed changes

jacobbogers and others added 2 commits June 2, 2019 16:08

[withWidth] Migrate to hooks

c4a264d

Josh review

0e59653

oliviertassinari approved these changes Jun 2, 2019

View reviewed changes

joshwooding approved these changes Jun 2, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label Jun 2, 2019

oliviertassinari removed their assignment Jun 2, 2019

oliviertassinari merged commit 942fbf2 into mui:master Jun 3, 2019

eps1lon reviewed Jun 8, 2019

View reviewed changes

oliviertassinari mentioned this pull request Jun 9, 2019

[docs] Revert unreleased changes to the useMediaQuery API #16127

Merged

fzaninotto mentioned this pull request Jun 12, 2019

Regression: withWidth fails on the server side because of useMediaQuery #16184

Closed

2 tasks

oliviertassinari mentioned this pull request Jun 23, 2019

[useMediaQuery] Accept function as argument & more #16343

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[withWidth] Migrate to hooks #15678

[withWidth] Migrate to hooks #15678

jacobbogers commented May 13, 2019 •

edited by oliviertassinari

Loading

jacobbogers commented May 13, 2019

eps1lon commented May 13, 2019

jacobbogers commented May 13, 2019 •

edited

Loading

jacobbogers commented May 13, 2019 •

edited

Loading

mui-pr-bot commented May 13, 2019 •

edited

Loading

eps1lon commented May 14, 2019

oliviertassinari commented May 14, 2019 •

edited

Loading

jacobbogers commented May 14, 2019 •

edited

Loading

oliviertassinari commented May 15, 2019

oliviertassinari commented May 26, 2019

oliviertassinari commented Jun 2, 2019

oliviertassinari Jun 2, 2019 •

edited

Loading

jacobbogers commented Jun 2, 2019 •

edited

Loading

oliviertassinari commented Jun 2, 2019

jacobbogers commented Jun 2, 2019

eps1lon Jun 8, 2019

oliviertassinari Jun 8, 2019

eps1lon Jun 8, 2019

oliviertassinari Jun 8, 2019

oliviertassinari Jun 8, 2019 •

edited

Loading

oliviertassinari Jun 8, 2019

oliviertassinari Jun 8, 2019

oliviertassinari Jun 8, 2019

henrylearn2rock commented Jun 19, 2019

joshwooding commented Jun 19, 2019

oliviertassinari commented Jun 20, 2019 •

edited

Loading


		[json2mq](https://github.com/akiran/json2mq) is used to generate media query string from a JavaScript object.

		{{"demo": "pages/components/use-media-query/JavaScriptMedia.js", "defaultCodeOpen": true}}

[withWidth] Migrate to hooks #15678

[withWidth] Migrate to hooks #15678

Conversation

jacobbogers commented May 13, 2019 • edited by oliviertassinari Loading

jacobbogers commented May 13, 2019

eps1lon commented May 13, 2019

jacobbogers commented May 13, 2019 • edited Loading

jacobbogers commented May 13, 2019 • edited Loading

mui-pr-bot commented May 13, 2019 • edited Loading

eps1lon commented May 14, 2019

oliviertassinari commented May 14, 2019 • edited Loading

jacobbogers commented May 14, 2019 • edited Loading

oliviertassinari commented May 15, 2019

oliviertassinari commented May 26, 2019

oliviertassinari commented Jun 2, 2019

oliviertassinari Jun 2, 2019 • edited Loading

Choose a reason for hiding this comment

jacobbogers commented Jun 2, 2019 • edited Loading

oliviertassinari commented Jun 2, 2019

jacobbogers commented Jun 2, 2019

eps1lon Jun 8, 2019

Choose a reason for hiding this comment

oliviertassinari Jun 8, 2019

Choose a reason for hiding this comment

eps1lon Jun 8, 2019

Choose a reason for hiding this comment

oliviertassinari Jun 8, 2019

Choose a reason for hiding this comment

oliviertassinari Jun 8, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jun 8, 2019

Choose a reason for hiding this comment

oliviertassinari Jun 8, 2019

Choose a reason for hiding this comment

oliviertassinari Jun 8, 2019

Choose a reason for hiding this comment

henrylearn2rock commented Jun 19, 2019

joshwooding commented Jun 19, 2019

oliviertassinari commented Jun 20, 2019 • edited Loading

jacobbogers commented May 13, 2019 •

edited by oliviertassinari

Loading

jacobbogers commented May 13, 2019 •

edited

Loading

jacobbogers commented May 13, 2019 •

edited

Loading

mui-pr-bot commented May 13, 2019 •

edited

Loading

oliviertassinari commented May 14, 2019 •

edited

Loading

jacobbogers commented May 14, 2019 •

edited

Loading

oliviertassinari Jun 2, 2019 •

edited

Loading

jacobbogers commented Jun 2, 2019 •

edited

Loading

oliviertassinari Jun 8, 2019 •

edited

Loading

oliviertassinari commented Jun 20, 2019 •

edited

Loading