Skip to content

Stats: use treeSelect for stats-list item's selectors #21507

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.

Sign up for GitHub

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

Merged
jsnajdr merged 2 commits into from
Jan 18, 2018
Merged

Stats: use treeSelect for stats-list item's selectors #21507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab32c40
Use treeSelect to memoize stats/lists selectors
jsnajdr Jan 17, 2018
464038e
Stats: improve prop comparison for StatModuleChartTabs and PostTrends…
jsnajdr Jan 17, 2018
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
7 changes: 6 additions & 1 deletion client/my-sites/stats/post-trends/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import i18n, { localize } from 'i18n-calypso';
/**
* Internal dependencies
*/
import compareProps from 'lib/compare-props';
import Month from './month';
import Card from 'components/card';
import SectionHeader from 'components/section-header';
Expand Down Expand Up @@ -200,7 +201,7 @@ class PostTrends extends React.Component {
}
}

export default connect( state => {
const mapStateToProps = state => {
const siteId = getSelectedSiteId( state );
const query = {
startDate: i18n
Expand All @@ -225,4 +226,8 @@ export default connect( state => {
query,
siteId,
};
};

export default connect( mapStateToProps, null, null, {
areStatePropsEqual: compareProps( { deep: [ 'query' ] } ),
Copy link
Contributor Author

@samouri samouri Jan 17, 2018

Choose a reason for hiding this comment

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

very cool -- it keeps the ugliness out of the component

} )( localize( PostTrends ) );
7 changes: 6 additions & 1 deletion client/my-sites/stats/stats-chart-tabs/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { localize } from 'i18n-calypso';
/**
* Internal dependencies
*/
import compareProps from 'lib/compare-props';
import ElementChart from 'components/chart';
import Legend from 'components/chart/legend';
import StatTabs from '../stats-tabs';
Expand Down Expand Up @@ -329,7 +330,11 @@ const connectComponent = connect(
siteId,
};
},
{ recordGoogleEvent }
{ recordGoogleEvent },
null,
{
areStatePropsEqual: compareProps( { deep: [ 'quickQuery', 'fullQuery' ] } ),
}
);

export default flowRight( localize, connectComponent )( StatModuleChartTabs );
81 changes: 31 additions & 50 deletions client/state/stats/lists/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
* External dependencies
*/

import { forOwn, get, reduce, isArray, map, flatten } from 'lodash';
import { get, reduce, isArray, map, flatten } from 'lodash';
import i18n from 'i18n-calypso';

/**
* Internal dependencies
*/
import createSelector from 'lib/create-selector';
import treeSelect from 'lib/tree-select';
import { getSerializedStatsQuery, normalizers, buildExportArray } from './utils';
import { getSite } from 'state/sites/selectors';

Expand Down Expand Up @@ -71,11 +71,11 @@ export function getSiteStatsForQuery( state, siteId, statType, query ) {
* @param {?Number} query.gmtOffset GMT offset of the queried site
* @return {Object} Parsed Data for the query
*/
export const getSiteStatsPostStreakData = createSelector(
( state, siteId, query ) => {
export const getSiteStatsPostStreakData = treeSelect(
( state, siteId, query ) => [ getSiteStatsForQuery( state, siteId, 'statsStreak', query ) ],
( [ streakData ], siteId, query ) => {
const gmtOffset = query.gmtOffset || 0;
const response = {};
const streakData = getSiteStatsForQuery( state, siteId, 'statsStreak', query );
// ensure streakData.data exists and it is not an array
if ( streakData && streakData.data && ! isArray( streakData.data ) ) {
Object.keys( streakData.data ).forEach( timestamp => {
Expand All @@ -92,10 +92,8 @@ export const getSiteStatsPostStreakData = createSelector(

return response;
},
( state, siteId, query ) => getSiteStatsForQuery( state, siteId, 'statsStreak', query ),
( state, siteId, query ) => {
const serializedQuery = getSerializedStatsQuery( query );
return [ siteId, 'statsStreak', serializedQuery ].join();
{
getCacheKey: ( siteId, query ) => [ siteId, getSerializedStatsQuery( query ) ].join(),
}
);

Expand All @@ -107,22 +105,12 @@ export const getSiteStatsPostStreakData = createSelector(
* @param {Object} query Stats query object
* @return {?Number} Max number of posts by day
*/
export const getSiteStatsMaxPostsByDay = createSelector(
( state, siteId, query ) => {
let max = 0;

forOwn( getSiteStatsPostStreakData( state, siteId, query ), count => {
if ( count > max ) {
max = count;
}
} );

return max || null;
},
( state, siteId, query ) => getSiteStatsForQuery( state, siteId, 'statsStreak', query ),
( state, siteId, query ) => {
const serializedQuery = getSerializedStatsQuery( query );
return [ siteId, 'statsStreakMax', serializedQuery ].join();
export const getSiteStatsMaxPostsByDay = treeSelect(
( state, siteId, query ) => [ getSiteStatsPostStreakData( state, siteId, query ) ],
( [ postStreakData ] ) =>
reduce( postStreakData, ( max, count ) => ( count > max ? count : max ), 0 ) || null,
Copy link
Contributor Author

@samouri samouri Jan 17, 2018

Choose a reason for hiding this comment

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

nice

{
getCacheKey: ( siteId, query ) => [ siteId, getSerializedStatsQuery( query ) ].join(),
}
);

Expand All @@ -134,20 +122,11 @@ export const getSiteStatsMaxPostsByDay = createSelector(
* @param {Object} query Stats query object
* @return {?Number} Max number of posts by day
*/
export const getSiteStatsTotalPostsForStreakQuery = createSelector(
( state, siteId, query ) => {
return reduce(
getSiteStatsPostStreakData( state, siteId, query ),
( posts, sum ) => {
return sum + posts;
},
0
);
},
( state, siteId, query ) => getSiteStatsForQuery( state, siteId, 'statsStreak', query ),
( state, siteId, query ) => {
const serializedQuery = getSerializedStatsQuery( query );
return [ siteId, 'statsStreakMax', serializedQuery ].join();
export const getSiteStatsTotalPostsForStreakQuery = treeSelect(
( state, siteId, query ) => [ getSiteStatsPostStreakData( state, siteId, query ) ],
( [ postStreakData ] ) => reduce( postStreakData, ( sum, posts ) => sum + posts, 0 ),
Copy link
Contributor Author

@samouri samouri Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

FFTI: we could even use the sum function here

Copy link
Member

@jsnajdr jsnajdr Jan 18, 2018

Choose a reason for hiding this comment

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

I'm going to remove the "total posts" and "max posts" selectors in a follow-up PR, as they are not really needed. I just didn't want to make this PR more complicated. And I already finished their treeSelect implementation when I realized they can be removed :(

{
getCacheKey: ( siteId, query ) => [ siteId, getSerializedStatsQuery( query ) ].join(),
}
);

Expand Down Expand Up @@ -175,19 +154,21 @@ export function getSiteStatsPostsCountByDay( state, siteId, query, date ) {
* @param {Object} query Stats query object
* @return {*} Normalized Data for the query, typically an array or object
*/
export const getSiteStatsNormalizedData = createSelector(
( state, siteId, statType, query ) => {
const data = getSiteStatsForQuery( state, siteId, statType, query );
if ( 'function' === typeof normalizers[ statType ] ) {
const site = getSite( state, siteId );
return normalizers[ statType ].call( this, data, query, siteId, site );
export const getSiteStatsNormalizedData = treeSelect(
( state, siteId, statType, query ) => [
getSiteStatsForQuery( state, siteId, statType, query ),
getSite( state, siteId ),
],
( [ siteStats, site ], siteId, statType, query ) => {
const normalizer = normalizers[ statType ];
if ( typeof normalizer !== 'function' ) {
return siteStats;
}
return data;
return normalizer( siteStats, query, siteId, site );
},
( state, siteId, statType, query ) => getSiteStatsForQuery( state, siteId, statType, query ),
( state, siteId, statType, query ) => {
const serializedQuery = getSerializedStatsQuery( query );
return [ siteId, statType, serializedQuery ].join();
{
getCacheKey: ( siteId, statType, query ) =>
[ siteId, statType, getSerializedStatsQuery( query ) ].join(),
}
);

Expand Down
8 changes: 4 additions & 4 deletions client/state/stats/lists/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ import { userState } from 'state/selectors/test/fixtures/user-state';

describe( 'selectors', () => {
beforeEach( () => {
getSiteStatsPostStreakData.memoizedSelector.cache.clear();
getSiteStatsMaxPostsByDay.memoizedSelector.cache.clear();
getSiteStatsTotalPostsForStreakQuery.memoizedSelector.cache.clear();
getSiteStatsNormalizedData.memoizedSelector.cache.clear();
getSiteStatsPostStreakData.clearCache();
Copy link
Contributor Author

@samouri samouri Jan 17, 2018

Choose a reason for hiding this comment

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

I think you could use jest.resetModules() here instead of clearCache.

Copy link
Member

@jsnajdr jsnajdr Jan 18, 2018

Choose a reason for hiding this comment

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

This is an awesome idea, but I'm afraid it requires nontrivial effort to make it work. Instead of importing all the tested selectors once at the top of the test file, we'd have to require them in each beforeEach call. Otherwise, the jest.resetModules() call doesn't have the desired effect.

It's definitely worth exploring in a standalone PR -- there is a lot of clearCache-style calls in our tests and they could be all avoided.

getSiteStatsMaxPostsByDay.clearCache();
getSiteStatsTotalPostsForStreakQuery.clearCache();
getSiteStatsNormalizedData.clearCache();
} );

describe( 'isRequestingSiteStatsForQuery()', () => {
Expand Down