Plugins Browser: Redux :allthethings: #8786

johnHackworth · 2016-10-17T16:10:28Z

Calypso's plugin browser was still using flux for its data. This PR migrates all the data structures used for it to our redux state tree.

A couple of disclaimers:

Sites are still being read from the old sites-list. The browser uses some selectors that are still not migrated. This will be updated once we migrate everything from sites-list.
there's some logic in the action that fetches the plugin lists from the API. It's the logic related with controlling when a page can be requested and when we should skip it (for example, because there's already a request for this page going on). I tried to add this logic to the state tree instead, but since we are using the infiniteScroll mixin for this, the way infiniteScroll triggers update events conflicts with the order where the data is updated in the state tree.
For example, once you request page 1, we dispatch an event to update the state tree value "lastPageFetched", so the next time we try to load the next page, we know that we should call the action for the second page... but it doesn't mind, because by then, infiniteScroll would already have called the 'fetchNextPage' method 10 times, all of the for the first page. Storing this "lastPageCalled" as a variable within the actions module allows us to increment it immediately after the action is called and not depend on the events firing in the correct order.

How to test

Go to http://calypso.localhost:3000/plugins/browse. You should see something like this
Browse the different categories. Make sure that next pages are loaded and new plugins are appended when you scroll down
Search for any word. Make sure that the results you get make sense and you can scroll down and load further pages.
Everything should work the same than it currently does in https://wordpress.com/plugins/browse

ping @enejb @ryelle @tyxla

matticbot · 2016-10-17T16:10:30Z

Test live: https://calypso.live/?branch=add/plugins-reduxify

tyxla

This is some great work!

Everything is working as described, and tests are passing ✅ .

I've found a bug with plugin results while testing: #8877. Good news - it was there before, so it's not caused by this PR. At first glance, it appears to be an API issue.

I've added some notes, mostly minor stuff. Good job 👍

tyxla · 2016-10-20T09:35:07Z

client/my-sites/plugins/plugins-browser/index.jsx

+import QueryPluginLists from 'components/data/query-plugin-lists';
+
+const SHORT_LIST_LENGTH = 6;
+const FIST_PAGE = 1;


There's a small typo here - should probably be FIRST_PAGE.

tyxla · 2016-10-20T12:41:06Z

client/components/data/query-plugin-lists/README.md

+```jsx
+function PluginBrowser() {
+	const categories = [ 'new', 'popular' ];
+	return <QueryPluginLists categories={ categories } />;


Probably too trivial, but let's also add the import of the QueryPluginLists component into the example code.

tyxla · 2016-10-20T12:49:12Z

client/state/plugins/wporg/actions.js

+				page: page,
+				category: category,
+				search: searchTerm
+			}, function( error, data ) {


How about using a Promise here? I can see that wporg.fetchPluginsList uses superagent, so that appears to be a good occasion for refactoring that method to use a Promise.

tyxla · 2016-10-20T12:51:23Z

client/state/plugins/wporg/actions.js

+					_fetchingLists[ category ] = null;
+					_totalPagesPerCategory[ category ] = data.info.pages;
+					dispatch( {
+						type: WPORG_PLUGIN_RECEIVE_LIST,


I generally prefer to dispatch 2 separate actions for success and failure - provides more granularity.

tyxla · 2016-10-20T12:52:12Z

client/state/plugins/wporg/reducer.js

+
+function updatePluginsList( state = defaultPluginsListState, category, page, list ) {
+	if ( ! page || page === _DEFAULT_FIRST_PAGE ) {
+		state.shortLists[ category ] = Object.assign( [], list.slice( 0, 6 ) );


I think we shouldn't be hardcoding 6 - how about importing SHORT_LIST_LENGTH here?

right! 👍

tyxla · 2016-10-20T12:55:36Z

client/state/plugins/wporg/test/actions.js

+				done();
+			}, 2 ) );
+		} );
+	} );


We could use some more action tests for all dispatched actions.

Yeah, you're right! let's add them in another PR to not further complicate this one 👍

tyxla · 2016-10-20T12:56:02Z

client/state/plugins/wporg/selectors.js

@@ -24,4 +24,36 @@ const isFetched = function( state, pluginSlug ) {
 	return !! plugin.fetched;
 };

-export default { getPlugin, isFetching, isFetched };
+const canFetchList = function( state, category, page, searchTerm ) {


It would be awesome to have tests for these selectors. We can do it in another PR too (I'd love to help with that).

Yeah, you're right! let's add them in another PR to not further complicate this one 👍

tyxla · 2016-10-20T12:57:02Z

client/state/plugins/wporg/reducer.js

@@ -1,13 +1,23 @@
 /**
+ * External dependencies


It would be awesome to have tests for the reducer. We can do it in another PR too (I'd love to help with that).

tyxla · 2016-10-20T13:04:56Z

client/state/plugins/wporg/actions.js

+			// We need to debounce this method to avoid mixing diferent dispatch batches (and get an invariant violation from react)
+			// Since the infinite scroll mixin is launching a bunch of fetch requests at the same time, without debounce is too easy
+			// to get two of those requests running at (almost) the same time and getting react to freak out.
+			_lastFetchedPagePerCategory[ category ] = typeof _lastFetchedPagePerCategory[ category ] === 'undefined'


We could simplify this by using Object.hasOwnProperty() here.

tyxla · 2016-10-20T13:08:08Z

client/state/plugins/wporg/selectors.js

+};
+
+const getList = function( state, category ) {
+	return state.plugins.wporg.lists.fullLists[ category ] ? state.plugins.wporg.lists.fullLists[ category ] : [];


We could use lodash's get to simplify these.

I really think that _.get is less legible than just using the proper object chain of properties, when possible

I'm fine with leaving it as-is, as long as there's no way state.plugins.wporg.lists.fullLists[ category ] would throw an error because a prop in the chain is not defined.

@johnHackworth why do you find get() less legible? the safety is provides us is very valuable, plus in this case it cuts the line in half and at a glance shows what happens on missing data…

const getList = ( state, category ) => get( state, `plugins.wporg.lists.fullLists[ ${ category } ]`, [] );

dmsnell · 2016-11-16T15:20:44Z

client/components/data/query-plugin-lists/index.jsx

+		props.categories.forEach( ( category ) => {
+			this.props.fetchPluginsList( category );
+		} );
+	}


Is there a reason we create request() if it's only ever called once?

alternatively, we could shorten this down…

props.categories.forEach( props.fetchPluginsList );

dmsnell · 2016-11-16T15:21:43Z

client/components/data/query-plugin-lists/index.jsx

+
+	request( props ) {
+		props.categories.forEach( ( category ) => {
+			this.props.fetchPluginsList( category );


warning here - we're using this.props on this line but using the passed-in props in the rest of the method

dmsnell · 2016-11-16T15:22:39Z

client/components/data/query-plugin-lists/index.jsx

+}
+
+QueryPluginLists.propTypes = {
+	categories: PropTypes.array.isRequired


might as well define what the array is of…

categories: PropTypes.arrayOf( PropTypes.string ).isRequired,

dmsnell · 2016-11-16T15:24:35Z

client/my-sites/plugins/plugins-browser/index.jsx

-			PluginsActions.fetchNextCategoryPage( this.props.category );
+	componentWillReceiveProps( nextProps ) {
+		if ( nextProps.search ) {
+			if ( nextProps.search !== this.props.search ) {


this nesting is redundant. even if nextProps.search is undefined it will successfully test for equality. also if we want to make sure we don't search for undefined then we can simply join the conditional

if ( nextProps.search && nextProps.search !== this.props.search ) { this.fetchNextPagePlugins( nextProps.search ); }

dmsnell · 2016-11-16T15:26:20Z

client/my-sites/plugins/plugins-browser/index.jsx

-	getPluginsFullList( listName ) {
-		return this.state.fullLists[ listName ] ? this.state.fullLists[ listName ].list : [];
+	fetchNextPagePlugins( searchTerm ) {
+		searchTerm = typeof searchTerm === 'string' ? searchTerm : this.props.search;


this seems like a logical error. above we were avoiding calling this function if the search term weren't available, but here we're handling that case. could this ever be called without a string parameter? should it be called without a string parameter? what does it mean in that case?

dmsnell · 2016-11-16T15:39:10Z

client/my-sites/plugins/plugins-browser/index.jsx

+			if ( this.props.canFetchList( this.props.category, lastFetchedPage + 1 ) ) {
+				this.props.fetchPluginsList( this.props.category, lastFetchedPage + 1 );
+			}
+		}


something about this is hard for me to parse. there is much nesting going on and I'm not sure the sequence of what should be happening is clear. could we flatten this down maybe?

const { canFetchList, category, currentSearchTerm, fetchPluginList, lastPage, } = this.props; if ( searchTerm && currentSearchTerm !== searchTerm ) { return fetchPluginList( 'search', FIRST_PAGE, searchTerm ); } // note we are coercing the boolean here to zero // out the function when less than FIRST PAGE const nextSearchPage = ( lastPage.search + 1 ) * ( lastPage.search >= FIRST_PAGE ); if ( searchTerm && canFetchList( 'search', nextSearchPage, searchTerm ) ) { return fetchPluginList( 'search', nextSearchPage, searchTerm ); } const nextCategoryPage = ( lastPage[ category ] + 1 ) * ( lastPage[ category ] >= FIRST_PAGE ); if ( category && canFetchList( category, nextCategoryPage ) ) { return fetchPluginsList( category, nextCategoryPage ); }

dmsnell · 2016-11-16T15:43:45Z

client/state/plugins/wporg/actions.js

@@ -2,18 +2,35 @@
 * External dependencies
 */
 const debug = require( 'debug' )( 'calypso:wporg-data:actions' );
-
+import debounce from 'lodash/debounce';


we're using direct imports from lodash and a Babel transform minimizes them appropriately

import { debounce } from 'lodash';

dmsnell · 2016-11-16T15:46:49Z

client/state/plugins/wporg/actions.js

 /**
 * Internal dependencies
 */
 import wporg from 'lib/wporg';
 import utils from 'lib/plugins/utils';
-import { WPORG_PLUGIN_DATA_RECEIVE, FETCH_WPORG_PLUGIN_DATA } from 'state/action-types';
+import { WPORG_PLUGIN_DATA_RECEIVE, FETCH_WPORG_PLUGIN_DATA, WPORG_PLUGIN_RECEIVE_LIST, WPORG_PLUGIN_FETCH_LIST } from 'state/action-types';
+import { _LIST_DEFAULT_SIZE, _DEFAULT_FIRST_PAGE } from './constants';


Please check in the style guide: we avoid leading underscores in names. This comment is valid for this entire file.

dmsnell · 2016-11-16T15:48:59Z

client/state/plugins/wporg/actions.js

+		return false;
+	}
+	return true;
+};


@johnHackworth our actions should all be pure functions which we can call at any time and get the same result. storing state here in the actions file is really going to confuse us and lead to bugs later on. we should be storing these values in the state tree and allowing the reducers to update them.

if you would like any help or review of the Redux flow and model, I would be more than happy to do a quick session with you and anyone else interested.

dmsnell · 2016-11-16T15:53:26Z

client/state/plugins/wporg/actions.js

+	},
+
+	fetchPluginsList: function( category, page, searchTerm ) {
+		return debounce( ( dispatch ) => {


let's be very careful here with debounce. let's add it when we realize we need it instead of in advance.

did you test to make sure this works? it looks like we won't actually be debouncing anything…

const f1 = fetchPluginsList( 'myCategory', 1, 'splines' ) // === debounced thunk const f2 = fetchPluginsList( 'myCategory', 1, 'splines' ) // === debounced thunk f1 === f2 // false - we have to separate debounced functions, thus no debouncing

dmsnell · 2016-11-16T15:54:14Z

client/state/plugins/wporg/actions.js

+
+			if ( ! canFetchList( category, page, searchTerm ) ) {
+				return;
+			}


should this not belong at the very top of this function?

dmsnell · 2016-11-16T15:56:35Z

client/state/plugins/wporg/actions.js

+			if ( searchTerm ) {
+				searchTerm = searchTerm.trim();
+				_currentSearchTerm = searchTerm;
+			}


why not make the search term default to the empty value for its given type, ( type, emptyValue ) => ( string, '' ), and then use the idempotent trim() operation on it?

// assume this code isn't actually modifying data in actions.js { currentSearchTerm: searchTerm.trim() }

no if is necessary

dmsnell · 2016-11-16T15:57:24Z

client/state/plugins/wporg/actions.js

+				type: WPORG_PLUGIN_FETCH_LIST,
+				page: page,
+				category: category,
+				searchTerm: searchTerm


with the object shorthand syntax this becomes…

{ type: WPORG_PLUGIN_FETCH_LIST, page, category, searchTerm, }

dmsnell · 2016-11-16T15:58:44Z

client/state/plugins/wporg/reducer.js

+	currentSearchTerm: null,
+	fetching: {},
+	lastFetchedPage: {}
+} );


I would like to challenge us to never create complex initial states like this. using combineReducers() gives us the ability to create compositions of very simple reducers, each of whose initial state is a simple value.

dmsnell · 2016-11-16T16:00:06Z

client/state/plugins/wporg/reducer.js

+
+function updatePluginsList( state = defaultPluginsListState, category, page, list ) {
+	if ( ! page || page === _DEFAULT_FIRST_PAGE ) {
+		state.shortLists[ category ] = Object.assign( [], list.slice( 0, 6 ) );


notice that by directly mutating state here we will be making a change without changing the value of state, thus all pure components which only update on state changes won't update as a result of this change.

instead, we need to be splitting reducers into different units and composing them together. I'd be more than happy to give a short demo/workshop

dmsnell · 2016-11-16T16:07:03Z

client/state/plugins/wporg/reducer.js

+	const { type, category, page, data } = action;
+	switch ( type ) {
+		case WPORG_PLUGIN_RECEIVE_LIST:
+			return Object.assign( {}, state, updatePluginsList( defaultPluginsListState, category, page, data ) );


I often see this pattern, and it's one of the first things that might come to mind when we are creating Redux reducers, but it's actually perpendicular to what we want to be using Redux for. This mangles the different pieces of data together when we want nice separation of data.

Imagine that instead of having one updater which updates five pieces of data that we had five updaters each updating their own simple piece of data in response to this same type.

const isFetching = ( state = {}, { type, category } ) => get( { [ WPORG_PLUGIN_RECEIVE_LIST ]: { ...state, [ category ]: false }, [ WPORG_PLUGIN_FETCH_LIST ]: { ...state, [ category ]: true }, }, type, state ); const lastFetchedPage = ( state = {}, { type, category, page } ) => get( { [ WPORG_PLUGINS_RECEIVE_LIST ]: { ...state, [ category ]: page }, }, type, state ); export default combineReducers( isFetching, lastFetchedPage, );

I only included a couple of the simple reducers, but hopefully you can see how we're removing the logic about those different data pieces from big reducers and splitting them into several small reducers. There's some redundancy in indicating that we're listening for specific action types, but that's the nature of the distributed and independent Redux core.

dmsnell · 2016-11-16T16:12:23Z

client/state/plugins/wporg/selectors.js

+		return false;
+	}
+
+	return true;


another way to look at this is a simple list where any truth value means the whole thing fails…

// if any of the rules below are true then we can't fetch the list const canFetchList = ( { plugins: { wporg: { lists } } }, category, page, searchTerm ) => ! [ searchTerm && lists.fetching.search ), lists.fetching[ category ], lists.lastFetchedPage[ category ] >= page, ].some( identity );

dmsnell · 2016-11-16T16:21:01Z

client/state/plugins/wporg/selectors.js

+	return state.plugins.wporg.lists.shortLists[ category ] ? state.plugins.wporg.lists.shortLists[ category ] : [];
+};
+
+export default { getPlugin, isFetching, isFetched, isFetchingList, canFetchList, getList, getShortList, getCurrentSearchTerm };


let's export each of these in the place they are defined instead of exporting the default export as an object.

export const getShortList = ( state, category ) => get( state, …, [] );

dmsnell · 2016-11-16T16:22:44Z

client/state/plugins/wporg/test/actions.js

+	describe( 'Plugin lists', function() {
+		it( 'Actions should have method fetchPluginData', function() {
+			assert.isFunction( WPorgActions.fetchPluginsList );
+		} );


some may argue this point and think differently than me, but I find these tests not too useful. I guess it does provide some constraints.

dmsnell · 2016-11-16T16:24:11Z

@johnHackworth I'm very excited to see some changes to the plugins to use Redux state instead of Flux stores and other data stores. I think there's quite a bit to do here before we could merge it, however, and some of that might just be seeing what it means to build inside of the Redux idiom. Please don't hesitate to ask any questions.

lancewillett · 2017-02-06T16:55:36Z

@johnHackworth This one hasn't had any activity in about 3 months. Should we close it, review and fix things and merge it in?

johnHackworth · 2017-02-06T19:41:34Z

@lancewillett well... I'm not sure. This was ~~almost ready to go~~ pretty advanced, but with quite some work left to be done, and also is very out of date now. I'll pass the ball to @enejb and @lezama , who are in command of this part of the codebase, to decide what to do with this :)

Anyway, if after AT we start wanting to change / improve the plugins browser (which is pretty possible) this migration to redux will be needed more sooner than later...

matticbot · 2017-02-21T12:47:03Z

@johnHackworth This PR needs a rebase

alisterscott · 2017-10-24T04:49:00Z

Closing as this PR is stale - please reopen if necessary

johnHackworth added 5 commits October 14, 2016 19:03

Plugins: Actions to redux

7db8ba4

Plugins: full list selectors

d2d4124

Plugins: Migrate shortlists to redux

7e1a6dc

Plugins: User query component to load plugins lists

ba4be29

Plugins: pagination on search

9d7e240

johnHackworth added Jetpack [Status] In Progress Jetpack Plugins labels Oct 17, 2016

johnHackworth self-assigned this Oct 17, 2016

matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Oct 17, 2016

lancewillett added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 17, 2016

johnHackworth added 4 commits October 18, 2016 12:17

Plugins: use accessError

33bcd3c

Plugins: remove all data lib

55b76d8

Plugins: Fix pagination in the browser

1684cb6

Plugins: Avoid showing 'no results' when we are still fetching

bb2f307

johnHackworth changed the title ~~Add/plugins reduxify~~ Plugins Browser: Redux :allthethings: Oct 19, 2016

johnHackworth added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Oct 19, 2016

Plugins: actions tests

ac2ba62

tyxla mentioned this pull request Oct 20, 2016

Jetpack Plugins: Some results are repeated on subsequent results pages #8877

Closed

tyxla reviewed Oct 20, 2016

View reviewed changes

dmsnell reviewed Nov 16, 2016

View reviewed changes

ockham mentioned this pull request Dec 6, 2016

Themes: Add queryRequestErrors reducer and tests #9839

Merged

matticbot added the [Status] Needs Rebase label Feb 21, 2017

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 30, 2017

mtias mentioned this pull request Sep 24, 2017

Framework: Redux single data tree and query components #5046

Closed

alisterscott closed this Oct 24, 2017

alisterscott deleted the add/plugins-reduxify branch October 24, 2017 04:49

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 24, 2017

Plugins Browser: Redux :allthethings: #8786

Plugins Browser: Redux :allthethings: #8786

Conversation

johnHackworth commented Oct 17, 2016 • edited Loading

How to test

matticbot commented Oct 17, 2016

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

dmsnell Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Nov 16, 2016

lancewillett commented Feb 6, 2017

johnHackworth commented Feb 6, 2017 • edited Loading

matticbot commented Feb 21, 2017

alisterscott commented Oct 24, 2017

johnHackworth commented Oct 17, 2016 •

edited

Loading

dmsnell Nov 16, 2016 •

edited

Loading

dmsnell Nov 16, 2016 •

edited

Loading

dmsnell Nov 16, 2016 •

edited

Loading

dmsnell Nov 16, 2016 •

edited

Loading

johnHackworth commented Feb 6, 2017 •

edited

Loading