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

[withWidth] Migrate to hooks #15678

Merged
merged 2 commits into from
Jun 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions docs/src/modules/utils/generateMarkdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,6 @@ function generateInheritance(reactAPI) {
suffix = ', from react-transition-group,';
break;

case 'EventListener':
suffix = ', from react-event-listener,';
break;

default:
break;
}
Expand Down
13 changes: 13 additions & 0 deletions docs/src/pages/components/use-media-query/JavaScriptMedia.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react';
import json2mq from 'json2mq';
import useMediaQuery from '@material-ui/core/useMediaQuery';

export default function JavaScriptMedia() {
const matches = useMediaQuery(
json2mq({
minWidth: 600,
}),
);

return <span>{`{ minWidth: 600 } matches: ${matches}`}</span>;
}
21 changes: 12 additions & 9 deletions docs/src/pages/components/use-media-query/UseWidth.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,23 @@ import useMediaQuery from '@material-ui/core/useMediaQuery';
import { createMuiTheme } from '@material-ui/core/styles';

/**
* Be extremely careful using this component. It only works because the number of
* Be careful using this hook. It only works because the number of
* breakpoints in theme is static. It will break once you change the number of
* breakpoints. See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
*/
function MyComponent() {
function useWidth() {
const theme = useTheme();
const width =
[...theme.breakpoints.keys].reverse().reduce((output, key) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const matches = useMediaQuery(theme.breakpoints.only(key));

return !output && matches ? key : output;
}, null) || 'xs';
const keys = [...theme.breakpoints.keys].reverse();
const queries = useMediaQuery(keys.map(key => theme.breakpoints.only(key)));
return (
queries.reduce((output, matches, index) => {
return !output && matches ? keys[index] : output;
}, null) || 'xs'
);
}

function MyComponent() {
const width = useWidth();
return <span>{`width: ${width}`}</span>;
}

Expand Down
37 changes: 16 additions & 21 deletions docs/src/pages/components/use-media-query/use-media-query.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,7 @@ Some of the key features:
You should provide a media query to the first argument of the hook.
The media query string can by any valid CSS media query, e.g. `'print'`.

```jsx
import useMediaQuery from '@material-ui/core/useMediaQuery';

function MyComponent() {
const matches = useMediaQuery('(min-width:600px)');

return <span>{`(min-width:600px) matches: ${matches}`}</span>;
}
```

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

## Using Material-UI's breakpoint helpers

Expand All @@ -49,6 +39,12 @@ function MyComponent() {

{{"demo": "pages/components/use-media-query/ThemeHelper.js"}}

## Using JavaScript syntax

[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}}
Copy link
Member

@oliviertassinari oliviertassinari Jun 2, 2019

Choose a reason for hiding this comment

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

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?


## Server-side rendering

An implementation of [matchMedia](https://developer.mozilla.org/en-US/docs/Web/API/Window/matchMedia) is required on the server, we recommend using [css-mediaquery](https://github.com/ericf/css-mediaquery).
Expand All @@ -59,19 +55,18 @@ We also encourage the usage of the `useMediaQueryTheme` version of the hook that
## Migrating from `withWidth()`

The `withWidth()` higher-order component injects the screen width of the page.
You can reproduce the same behavior as follow:
You can reproduce the same behavior with a `useWidth` hook:

```jsx
function MyComponent() {
function useWidth() {
const theme = useTheme();
const width =
[...theme.breakpoints.keys].reverse().reduce((output, key) => {
const matches = useMediaQuery(theme.breakpoints.only(key));

return !output && matches ? key : output;
}, null) || 'xs';

return <span>{width}</span>;
const keys = [...theme.breakpoints.keys].reverse();
const queries = useMediaQuery(keys.map(key => theme.breakpoints.only(key)));
return (
queries.reduce((output, matches, index) => {
return !output && matches ? keys[index] : output;
}, null) || 'xs'
);
}
```

Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@
"gm": "^1.23.0",
"isomorphic-fetch": "^2.2.1",
"jsdom": "^15.0.0",
"json2mq": "^0.2.0",
"jsonlint": "^1.6.3",
"jss-plugin-template": "^10.0.0-alpha.12",
"jss-rtl": "^0.2.1",
Expand Down
6 changes: 2 additions & 4 deletions packages/material-ui/src/Hidden/HiddenJs.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ describe('<HiddenJs />', () => {
const props = { [prop]: upDownOnly === 'only' ? breakpoint : true };

it(descriptions[upDownOnly], () => {
let wrapper = shallow(
const wrapper = shallow(
<HiddenJs width={width} {...props}>
<div>foo</div>
</HiddenJs>,
);
wrapper = wrapper.find('HiddenJs').shallow();
assert.strictEqual(wrapper.type(), null, 'should render null');
});
});
Expand All @@ -51,12 +50,11 @@ describe('<HiddenJs />', () => {
const props = { [prop]: upDownOnly === 'only' ? breakpoint : true };

it(descriptions[upDownOnly], () => {
let wrapper = shallow(
const wrapper = shallow(
<HiddenJs width={width} {...props}>
<div>foo</div>
</HiddenJs>,
);
wrapper = wrapper.find('HiddenJs').shallow();
assert.isNotNull(wrapper.type(), 'should render');
assert.strictEqual(wrapper.name(), 'div');
assert.strictEqual(wrapper.first().text(), 'foo', 'should render children');
Expand Down
38 changes: 26 additions & 12 deletions packages/material-ui/src/useMediaQuery/useMediaQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,40 +3,54 @@ import React from 'react';
// This variable will be true once the server-side hydration is completed.
let hydrationCompleted = false;

function deepEqual(a, b) {
return a.length === b.length && a.every((item, index) => item === b[index]);
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Capture d’écran 2019-06-08 à 15 24 29

doesn't support changing the number of queries provided.

It should be supported.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Jun 8, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I will give it a second try.

Copy link
Member

Choose a reason for hiding this comment

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

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.

let queries = multiple ? queryInput : [queryInput];
queries = queries.map(query => query.replace('@media ', ''));

const { defaultMatches = false, noSsr = false, ssrMatchMedia = null } = options;

const [matches, setMatches] = React.useState(() => {
if (hydrationCompleted || noSsr) {
return window.matchMedia(query).matches;
return queries.map(query => window.matchMedia(query).matches);
}
if (ssrMatchMedia) {
return ssrMatchMedia(query).matches;
return queries.map(query => ssrMatchMedia(query).matches);
}

// Once the component is mounted, we rely on the
// event listeners to return the correct matches value.
return defaultMatches;
return queries.map(() => defaultMatches);
});

React.useEffect(() => {
hydrationCompleted = true;

const queryList = window.matchMedia(query);
setMatches(queryList.matches);
const queryLists = queries.map(query => window.matchMedia(query));
setMatches(prev => {
const next = queryLists.map(queryList => queryList.matches);
return deepEqual(prev, next) ? prev : next;
});

function handleMatchesChange(event) {
setMatches(event.matches);
function handleMatchesChange() {
setMatches(queryLists.map(queryList => queryList.matches));
}

queryList.addListener(handleMatchesChange);
queryLists.forEach(queryList => {
queryList.addListener(handleMatchesChange);
});
return () => {
queryList.removeListener(handleMatchesChange);
queryLists.forEach(queryList => {
queryList.removeListener(handleMatchesChange);
});
};
}, [query]);
}, queries); // eslint-disable-line react-hooks/exhaustive-deps

return matches;
return multiple ? matches : matches[0];
}

export function testReset() {
Expand Down
49 changes: 28 additions & 21 deletions packages/material-ui/src/useMediaQuery/useMediaQuery.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,33 @@ import { assert } from 'chai';
import { spy } from 'sinon';
import useMediaQuery, { testReset } from './useMediaQuery';

function createMatchMedia(width, listeners) {
return query => ({
matches: mediaQuery.match(query, {
width,
}),
addListener: listener => {
listeners.push(listener);
},
removeListener: listener => {
const index = listeners.indexOf(listener);
if (index > -1) {
listeners.splice(index, 1);
}
},
});
function createMatchMedia(width, ref) {
const listeners = [];
return query => {
const instance = {
matches: mediaQuery.match(query, {
width,
}),
addListener: listener => {
listeners.push(listener);
},
removeListener: listener => {
const index = listeners.indexOf(listener);
if (index > -1) {
listeners.splice(index, 1);
}
},
};
ref.push({
instance,
listeners,
});
return instance;
};
}

describe('useMediaQuery', () => {
const listeners = [];
let matchMediaInstances;
let mount;
let values;

Expand All @@ -43,7 +51,8 @@ describe('useMediaQuery', () => {
beforeEach(() => {
testReset();
values = spy();
window.matchMedia = createMatchMedia(1200, listeners);
matchMediaInstances = [];
window.matchMedia = createMatchMedia(1200, matchMediaInstances);
});

after(() => {
Expand Down Expand Up @@ -193,11 +202,9 @@ describe('useMediaQuery', () => {
assert.strictEqual(values.callCount, 1);
assert.strictEqual(text(), 'false');

window.matchMedia = createMatchMedia(30000, listeners);
act(() => {
listeners[0]({
matches: true,
});
matchMediaInstances[0].instance.matches = true;
matchMediaInstances[0].listeners[0]();
});
assert.strictEqual(text(), 'true');
assert.strictEqual(values.callCount, 2);
Expand Down
16 changes: 2 additions & 14 deletions packages/material-ui/src/withMobileDialog/withMobileDialog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,7 @@ describe('withMobileDialog', () => {
foo
</ResponsiveDialog>,
);
assert.strictEqual(
wrapper
.find('WithMobileDialog')
.shallow()
.props().fullScreen,
true,
);
assert.strictEqual(wrapper.props().fullScreen, true);
});
});
}
Expand All @@ -43,13 +37,7 @@ describe('withMobileDialog', () => {
foo
</ResponsiveDialog>,
);
assert.strictEqual(
wrapper
.find('WithMobileDialog')
.shallow()
.props().fullScreen,
false,
);
assert.strictEqual(wrapper.props().fullScreen, false);
});
});
}
Expand Down
Loading