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

Lodash: Remove completely from @wordpress/viewport package #44572

Merged
merged 8 commits into from
Oct 5, 2022
Merged
Show file tree
Hide file tree
Changes from 7 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
3 changes: 1 addition & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions packages/jest-preset-default/scripts/setup-globals.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ global.window.cancelIdleCallback = function cancelIdleCallback( handle ) {
global.window.matchMedia = () => ( {
matches: false,
addListener: () => {},
addEventListener: () => {},
removeListener: () => {},
removeEventListener: () => {},
} );

// Setup fake localStorage.
Expand Down
2 changes: 2 additions & 0 deletions packages/react-native-editor/src/globals.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ if ( ! global.window.matchMedia ) {
global.window.matchMedia = () => ( {
matches: false,
addListener: () => {},
addEventListener: () => {},
removeListener: () => {},
removeEventListener: () => {},
} );
}

Expand Down
3 changes: 1 addition & 2 deletions packages/viewport/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@
"dependencies": {
"@babel/runtime": "^7.16.0",
"@wordpress/compose": "file:../compose",
"@wordpress/data": "file:../data",
"lodash": "^4.17.21"
"@wordpress/data": "file:../data"
},
"peerDependencies": {
"react": "^17.0.0"
Expand Down
38 changes: 14 additions & 24 deletions packages/viewport/src/listener.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { reduce, mapValues } from 'lodash';

/**
* WordPress dependencies
*/
Expand All @@ -21,7 +16,9 @@ const addDimensionsEventListener = ( breakpoints, operators ) => {
*/
const setIsMatching = debounce(
() => {
const values = mapValues( queries, ( query ) => query.matches );
const values = Object.fromEntries(
queries.map( ( [ key, query ] ) => [ key, query.matches ] )
);
dispatch( store ).setIsMatching( values );
},
0,
Expand All @@ -37,24 +34,17 @@ const addDimensionsEventListener = ( breakpoints, operators ) => {
*
* @type {Object<string,MediaQueryList>}
*/
const queries = reduce(
breakpoints,
( result, width, name ) => {
Object.entries( operators ).forEach(
( [ operator, condition ] ) => {
const list = window.matchMedia(
`(${ condition }: ${ width }px)`
);
list.addListener( setIsMatching );

const key = [ operator, name ].join( ' ' );
result[ key ] = list;
}
);

return result;
},
{}
const operatorEntries = Object.entries( operators );
const queries = Object.entries( breakpoints ).flatMap(
( [ name, width ] ) => {
return operatorEntries.map( ( [ operator, condition ] ) => {
const list = window.matchMedia(
`(${ condition }: ${ width }px)`
);
list.addEventListener( 'change', setIsMatching );
return [ `${ operator } ${ name }`, list ];
} );
}
);

window.addEventListener( 'orientationchange', setIsMatching );
Expand Down
24 changes: 9 additions & 15 deletions packages/viewport/src/listener.native.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* External dependencies
*/
import { reduce } from 'lodash';
import { Dimensions } from 'react-native';

/**
Expand All @@ -25,21 +24,16 @@ const matchWidth = ( operator, breakpoint ) => {
};

const addDimensionsEventListener = ( breakpoints, operators ) => {
const operatorEntries = Object.entries( operators );
const breakpointEntries = Object.entries( breakpoints );

const setIsMatching = () => {
const matches = reduce(
breakpoints,
( result, width, name ) => {
Object.entries( operators ).forEach(
( [ operator, condition ] ) => {
const key = [ operator, name ].join( ' ' );
result[ key ] = matchWidth( condition, width );
}
);

return result;
},
{}
);
const matches = breakpointEntries.flatMap( ( [ name, width ] ) => {
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing Object.fromEntries here. The setIsMatching action expects an object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good find. Would have been useful to have a test to catch this.

Addressed in 92e074e

return operatorEntries.map( ( [ operator, condition ] ) => [
`${ operator } ${ name }`,
matchWidth( condition, width ),
] );
} );

dispatch( store ).setIsMatching( matches );
};
Expand Down
32 changes: 15 additions & 17 deletions packages/viewport/src/with-viewport-match.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { mapValues } from 'lodash';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -36,19 +31,22 @@ import {
* @return {Function} Higher-order component.
*/
const withViewportMatch = ( queries ) => {
const queryEntries = Object.entries( queries );
const useViewPortQueriesResult = () =>
mapValues( queries, ( query ) => {
let [ operator, breakpointName ] = query.split( ' ' );
if ( breakpointName === undefined ) {
breakpointName = operator;
operator = '>=';
}
// Hooks should unconditionally execute in the same order,
// we are respecting that as from the static query of the HOC we generate
// a hook that calls other hooks always in the same order (because the query never changes).
// eslint-disable-next-line react-hooks/rules-of-hooks
return useViewportMatch( breakpointName, operator );
} );
Object.fromEntries(
queryEntries.map( ( [ key, query ] ) => {
let [ operator, breakpointName ] = query.split( ' ' );
if ( breakpointName === undefined ) {
breakpointName = operator;
operator = '>=';
}
// Hooks should unconditionally execute in the same order,
// we are respecting that as from the static query of the HOC we generate
// a hook that calls other hooks always in the same order (because the query never changes).
// eslint-disable-next-line react-hooks/rules-of-hooks
return [ key, useViewportMatch( breakpointName, operator ) ];
} )
);
return createHigherOrderComponent( ( WrappedComponent ) => {
return pure( ( props ) => {
const queriesResult = useViewPortQueriesResult();
Expand Down
25 changes: 12 additions & 13 deletions packages/viewport/src/with-viewport-match.native.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { mapValues } from 'lodash';

/**
* WordPress dependencies
*/
Expand All @@ -27,24 +22,28 @@ import { store } from './store';
*
* ```jsx
* function MyComponent( { isMobile } ) {
* return (
* <div>Currently: { isMobile ? 'Mobile' : 'Not Mobile' }</div>
* );
* return (
* <div>Currently: { isMobile ? 'Mobile' : 'Not Mobile' }</div>
* );
* }
*
* MyComponent = withViewportMatch( { isMobile: '< small' } )( MyComponent );
* ```
*
* @return {Function} Higher-order component.
*/
const withViewportMatch = ( queries ) =>
createHigherOrderComponent(
const withViewportMatch = ( queries ) => {
const queryEntries = Object.entries( queries );
return createHigherOrderComponent(
withSelect( ( select ) => {
return mapValues( queries, ( query ) => {
return select( store ).isViewportMatch( query );
} );
return Object.fromEntries(
queryEntries.map( ( [ key, query ] ) => {
return [ key, select( store ).isViewportMatch( query ) ];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the select( store ) call could be extracted out of the loop, too, if you think it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'm leaning towards keeping it as-is. Do you see any potential big performance improvements from that change?

Copy link
Member

Choose a reason for hiding this comment

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

Not really, it's just a double map lookup, equivalent to registry[ store ][ method ]. On the other hand, Object.entries is in a different ballpark, it would allocate and deallocate memory on each loop iteration.

} )
);
} ),
'withViewportMatch'
);
};

export default withViewportMatch;