Try wp.data store for orders #166

psealock · 2018-07-06T04:43:02Z

This is an example of how we might use wp.data for handling wc-admin data needs.

Test

Go to /wp-admin/admin.php?page=woodash#/analytics/orders
See all orders populated
Toggle status between completed and processing
See the network requests at each step

The good

Makes use of existing code and infrastructure.
Registering the store with Gutenberg's state tree allows for selectors and actions to be easily accessed within wc-admin or any extension.

const { getOrders } = select( 'woo-dash' );

built in withRehydration functions for storing and reading from localStorage

The not so good

Gutenberg writes its own middleware so that dispatched actions can return functions instead of only objects. GutenStore doesn't seem to be compatible with Redux-Thunk middleware, so we'd probably also need to write our own. I've quickly done this in this code as an example.
We'd need to write a method of storing and retrieving paginated results similar to what was done in Calypso.
Boilerplate code around reducers would need to be written also, including pending and failed requests.

coderkevin

In all, I think this is a fairly good implementation, but I wonder what it's getting us that withApiData didn't. I guess it pulls the API paths out of the components, which is good, at least.

I wonder how this approach will scale over time, and I also wonder how much of this code we'd have to change if we tried to run it within Calypso or some other environment.

coderkevin · 2018-07-09T21:00:24Z

client/analytics/report/orders/index.js

+}
+
+export default compose(
+	withSelect( select => {


I like how clean this function is.

coderkevin · 2018-07-09T21:01:39Z

client/analytics/report/orders/index.js

+		};
+	} ),
+	withDispatch( dispatch => {
+		return {


I'm not sure how I feel about having component logic here. If we do this a lot, the compose function will get pretty crowded.

Agree here. This is where the need to write our own middleware comes into play so that we can write thunks. Ideally it would look like this:

withDispatch( dispatch => { return { onToggleStatus: dispatch( 'woo-dash' ).toggleOrderStatus, }; } )

@aduth is this where you see the resolvers at work?

this is where I see requestHttpData() coming into play since it can trigger side-effects and through Redux update state on those transitions without breaking from the synchronous paradigm or resorting to thunks

coderkevin · 2018-07-09T21:02:27Z

client/analytics/report/orders/index.js

+		return {
+			onToggleStatus: function( order ) {
+				const status = order.status === 'completed' ? 'processing' : 'completed';
+				order.status = status;


Should we be mutating what is passed in here? Could this cause unforeseen problems?

no, and then yes 😉

coderkevin · 2018-07-09T21:06:20Z

client/analytics/report/orders/store/index.js

+				};
+			case 'UPDATE_ORDER':
+				const orderIndex = state.orders.findIndex( order => order.id === action.order.id );
+				const orders = state.orders.map( o => o );


Looks like this isn't actually mapping anything. Maybe const orders = { ...state.orders } or const orders = Object.assign( {}, state.orders ); instead?

coderkevin · 2018-07-09T21:09:11Z

client/analytics/report/orders/store/index.js

+					orders: action.orders,
+				};
+			case 'UPDATE_ORDER':
+				const orderIndex = state.orders.findIndex( order => order.id === action.order.id );


Array.prototype.findIndex isn't supported by IE, so this may need a polyfill if we want any IE11 support. Or use a lodash equivalent instead.

Also, I wonder if storing orders by id might be more optimal, then the findIndex wouldn't be necessary at all.

coderkevin · 2018-07-09T21:10:50Z

client/analytics/report/orders/store/index.js

+		return state;
+	},
+
+	actions: {


Note on @wordpress/data: I'm not a huge fan of this. In order to implement all of the WooCommerce API, we'll need a lot of these actions and they will all be very repetitive.

@coderkevin do you think there would be a reasonably way to generate these structures given a helper function? for example, with the structural similarity between endpoints and freshData could you have a sort of builder function given a type or kind of API data?

coderkevin · 2018-07-09T21:12:03Z

client/analytics/report/orders/store/index.js

+	resolvers: {
+		async getOrders() {
+			try {
+				const orders = await apiRequest( { path: '/wc/v2/orders' } );


Implementation note: While the path is simple here, after it's been copy/pasted to all endpoints, it may be annoying to update API version numbers in the future.

@aduth is this a good place for the apiRequest middleware? send /orders or some version-less path then have the middleware add the path?

with Calypso's data layer we'd simply intercept the WPCOM_HTTP_REQUEST or RAW_HTTP_REQUEST actions with custom Redux middleware and change the path property before passing along the action…

@aduth is this a good place for the apiRequest middleware? send /orders or some version-less path then have the middleware add the path?

Since apiRequest is a singleton working in a global space, adding a middleware which prefixes the WooCommerce-specific path prefixes could be problematic.

Maybe apiRequest here could point to some internal module which just proxies to the underlying @wordpress/api-request with the prefix attached?

coderkevin · 2018-07-09T21:12:53Z

client/analytics/report/orders/store/index.js

+				const orders = await apiRequest( { path: '/wc/v2/orders' } );
+				dispatch( 'woo-dash' ).setOrders( orders );
+			} catch ( error ) {
+				alert( error.responseJSON.message );


Obviously temporary code here, but we should be careful to not assume every error has a responseJSON

coderkevin · 2018-07-09T21:14:12Z

client/analytics/report/orders/store/mutations.js

+ */
+import apiRequest from '@wordpress/api-request';
+
+export async function requestUpdateOrder( dispatch, order ) {


I like how fairly simple this is. I kind of wish it were in the same place as the rest of the orders stuff though.

Yup, should be an action, along with others

timmyc

One quick comment from me, testing out nicely. Thanks for exploring this route Paul

timmyc · 2018-07-09T20:14:11Z

client/analytics/report/orders/store/index.js

+	resolvers: {
+		async getOrders() {
+			try {
+				const orders = await apiRequest( { path: '/wc/v2/orders' } );


any reason to not use /wc/v3 here?

What are the requirements for v3? I'm running WooCommerce version 3.4.3

@psealock There is a feature branch but it is behind master now.

Installing wc-api-dev should get you the endpoints in a separate plugin. Though if development is happening in the feature branch -- we should make sure to either keep that up to date, or have v3 in master. cc @claudiulodro

I've added a mention of this to the README for now https://github.com/woocommerce/wc-admin#prerequisites

The one in the WC master branch is going to have to be the development version while we finish up the v3 API, as we're doing some refactoring in the old API versions as part of it.

I would use wc-api-dev until we get the first stats endpoints done instead of the WC branch, as wc-api-dev is currently stable and the branch is currently in flux with many changes happening and some known bugs.

timmyc · 2018-07-09T20:14:44Z

client/analytics/report/orders/store/mutations.js

+import apiRequest from '@wordpress/api-request';
+
+export async function requestUpdateOrder( dispatch, order ) {
+	// Lets be optimistic


psealock · 2018-07-10T02:52:36Z

Thanks for the reviews @coderkevin @timmyc

I adjusted according to feedback. The most important update is some additional middleware so that thunks can be used to clean up how actions are used.

Now, you can create an action creator when registering your store

const store = registerStore( 'woo-dash', {
	reducer(){ ... },
	actions: {
		doAsync() {
			return requestUpdateOrder () => {
				dispatch( 'woo-dash' ).updateOrder( order );
				try {
					const updatedOrder = await apiRequest( {
						path: '/wc/v2/orders/' + order.id,
						method: 'PUT',
						data: order,
					} );

					dispatch( 'woo-dash' ).updateOrder( updatedOrder );
				} catch ( error ) {
					alert( error.responseJSON.message );
				}
			};
		}
	},
	selectors: {},
	resolvers: {},
}

And access it like you would any other action

withDispatch( dispatch => {
	return {
		requestUpdateOrder: function( updatedOrder ) {
			dispatch( 'woo-dash' ).requestUpdateOrder( updatedOrder );
		},
	};
} )

I quickly wrote some middleware logic without test or anything. I also ignored a couple comments for simplicity for readability, but will definitely address these things should we move forward with this branch. I think its is ready for a side by side comparison of fresh-data, but if there's anything else we'd like to see, I'm happy to add that in as well.

ryelle

I like the way this is going, but I think we should set up some standards - naming and file/folder structure - and stick to them here before we go too far in any direction. I'm thinking things like state structure, action/selector naming patterns, and separating out reducers/actions into object-type subfolders, etc

(I also didn't review the code in OrdersReport, since i assume it's just placeholder)

ryelle · 2018-07-10T16:39:50Z

client/analytics/report/orders/store/index.js

+	orderIds: [],
+};
+
+const store = registerStore( 'woo-dash', {


What happens when we start adding other object-types to this? Presumably we'll be tracking products too, customers, reviews, categories, etc. Can we still use things like combineReducers, to keep reducers organized?

And (eventually) for extensions, would they just register a different store?

Can we still use things like combineReducers, to keep reducers organized?

Yes! I will add some structure in today to address this and the points made in your parent comment

And (eventually) for extensions, would they just register a different store?

Yup, thats right. In fact, while registering their own store, Gutenberg's dispatch will give them access to selectors we create here via dispatch( 'woo-dash' )...

ryelle · 2018-07-10T16:40:40Z

client/analytics/report/orders/store/index.js

+		return state;
+	},
+
+	actions: {


I have the same question about actions + more object types, but it's easier to see this as a list of imported functions at least.

It looks like actions and selectors will need to be simple flat objects with uniquely named functions.

psealock · 2018-07-11T03:19:07Z

@ryelle I've made a first pass at organizing folders and structures, including using combineReducers to keep some order to the state tree

coderkevin · 2018-07-11T03:36:06Z

Thanks for taking this on @psealock!

youknowriad · 2018-07-14T00:21:58Z

Great to see the data module being explored.

A small not on the "thunk" and action creators in general. We're not satisfied with the way we create/use these middlewares in the data module and we're looking at implementing built-in support for async actions so users of the data module don't have to care about these middlewares and just write their actions (similarily to the resolvers).

Related WordPress/gutenberg#6999 WordPress/gutenberg#7546

dmsnell · 2018-07-15T20:44:08Z

We're not satisfied with the way we create/use these middlewares in the data module and we're looking at implementing built-in support for async actions so users of the data module don't have to care about these middlewares and just write their actions (similarily to the resolvers).

Thanks for chiming in @youknowriad - I'm really curious about discussing the paradigm in Calypso's data layer for this stuff. I'm biased since I designed it but I feel like it's a good fit for all this work without the complexity of thunks/generators/implicit properties. I guess it mainly boils down to "everything is data" but using the action-watchers middleware alongside the http middleware, where action watchers dispatch new HTTP actions in order to fulfill data requests and updates.

requestHttpData() is able to build on this system without being any different than the more verbose data layer handlers on account of the "everything is data" approach, which means middleware and transforms are still trivial.

the only thing I have yet to include that I think is important is giving components the ability to temporarily inject their own action watches: maybe this UI panel wants to toss around a floating star every time a post is liked, for example. we should be able to do so by watching the POST_LIKE action and now it's agnostic from network-level issues. if everything is written correctly then it won't matter if the like was triggered locally or at the remote end.

hope this doesn't sound like rambling.

timmyc · 2018-07-20T15:32:12Z

@psealock given the decision to move forward with this approach - do you mind giving this branch a rebase - and are there any other changes you would like to make before we merge this in?

psealock · 2018-07-24T08:11:27Z

This one is rebased against master. Its an overall fairly simplistic example, but a good starting off point for exploring how to make wp.data suit our needs.

Any objections, fixes, comments before merging?

timmyc

Few comments, but I agree this is a great launching point for us with wp.data and would love to get it merged in! When trying to test just now, I'm getting an empty screen and the following error:

index.js?ver=1532447154:8835 Uncaught Error: Cannot find module 'redux'
    at webpackMissingModule (index.js?ver=1532447154:8835)
    at Module../client/store/index.js (index.js?ver=1532447154:8835)
    at __webpack_require__ (bootstrap:19)
    at Module../client/index.js (index.js?ver=1532447154:5690)
    at __webpack_require__ (bootstrap:19)
    at bootstrap:83
    at bootstrap:83

I tried blowing away my node_modules directory.

timmyc · 2018-07-24T15:14:56Z

client/analytics/report/orders/store/actions.js

+
+				dispatch( 'wc-admin' ).updateOrder( updatedOrder );
+			} catch ( error ) {
+				if ( error && error.responseJSON ) {


I think this is fine for now, but we will want to come up with a standard way to handle these sorts of exceptions.

timmyc · 2018-07-24T15:16:16Z

client/analytics/report/orders/store/resolvers.js

+export default {
+	async getOrders() {
+		try {
+			const orders = await apiRequest( { path: '/wc/v3/orders' } );


This will also need to handle query args correct?

Right, it will need to handle those as well. @coderkevin is looking into incorporating some features of fresh-data into wp.data, hopefully including using query args as keys for retrieving already fetched data. I think its a good idea to revisit this as his efforts are further along

psealock · 2018-07-25T01:14:46Z

@timmyc updated to Gutenberg 3.3. It should work now

coderkevin · 2018-07-25T06:24:56Z

client/analytics/report/orders/store/actions.js

@@ -0,0 +1,42 @@
+/** @format */


If this is to be merged in for reals, store should be under client instead.

@psealock seems like a valid point, should we move this over?

psealock · 2018-07-26T23:48:25Z

orders related store functions have been moved under client/store

timmyc

Thanks for the updates @psealock and all your explorations here 🙌

Update to Gutenberg 3.3

psealock added the [Status] Needs Review label Jul 9, 2018

psealock requested a review from a team July 9, 2018 04:52

coderkevin reviewed Jul 9, 2018

View reviewed changes

timmyc reviewed Jul 9, 2018

View reviewed changes

ryelle reviewed Jul 10, 2018

View reviewed changes

coderkevin requested a review from dmsnell July 11, 2018 05:01

coderkevin added the [Type] Try label Jul 19, 2018

psealock force-pushed the try/wp-data-orders branch from dd5eba6 to 3b32ac7 Compare July 24, 2018 08:07

timmyc reviewed Jul 24, 2018

View reviewed changes

psealock force-pushed the try/wp-data-orders branch from 3b32ac7 to adec335 Compare July 25, 2018 01:13

coderkevin reviewed Jul 25, 2018

View reviewed changes

psealock force-pushed the try/wp-data-orders branch from d07c4f0 to bbac546 Compare July 26, 2018 23:47

timmyc approved these changes Jul 28, 2018

View reviewed changes

timmyc added [Status] Ready to Merge and removed [Status] Needs Review labels Jul 28, 2018

Use wp.data on Orders report

c361948

Update to Gutenberg 3.3

psealock force-pushed the try/wp-data-orders branch from bbac546 to c361948 Compare July 29, 2018 23:18

psealock merged commit 79bba3d into master Jul 29, 2018

psealock deleted the try/wp-data-orders branch July 29, 2018 23:23

Try wp.data store for orders #166

Try wp.data store for orders #166

Conversation

psealock commented Jul 6, 2018 • edited Loading

Test

The good

The not so good

coderkevin 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

timmyc 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

psealock commented Jul 10, 2018

ryelle 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

psealock commented Jul 11, 2018

coderkevin commented Jul 11, 2018

youknowriad commented Jul 14, 2018

dmsnell commented Jul 15, 2018

timmyc commented Jul 20, 2018

psealock commented Jul 24, 2018

timmyc 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

psealock commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock commented Jul 26, 2018

timmyc left a comment

Choose a reason for hiding this comment

psealock commented Jul 6, 2018 •

edited

Loading