Add redux-multi.

[justinshreve]

This PR adds redux-multi to our middleware for firing multiple actions from one action creator.

I have a need to take state and convert that into multiple add actions. After talking with Kevin, it looks like redux-multi is a good choice for this.

This is related to #12684 and our work on product & variation management for WooCommerce (that PR is WIP and is going to be split up): A set of variation types (think color, size, etc) and their values, as well as existing variations, will be used to generate new product variations. The action creator that will do this will return multiple add variation calls for each new one that needs to be created.

make test correctly passes & I tested a couple different parts of Calypso.

[matticbot]

Test live: https://calypso.live/?branch=add/redux-multi

[umurkontaci]

redux-multi is a 6-line code which can even be a one liner: https://github.com/ashaffer/redux-multi/blob/master/src/index.js

Do we really need to add an npm dependency for this?

On another note, I'm not very keen on an action creating multiple actions almost in a cascading manner.

Is there another way of doing this?

[aduth]

Is there another way of doing this?

Not sure I like it, but a thunk could very easily do something like this:

function doMultiple() {
	return ( dispatch ) => {
		dispatch( /* ... */ );
		dispatch( /* ... */ );
		dispatch( /* ... */ );
	};
}

Alternatively:

• Simply dispatch multiple times from the component / source
• Consolidate multiple dispatches into single action
• Refactor reducer(s) to handle single action as affecting multiple subtrees

[coderkevin]

Hi @umurkontaci,

redux-multi is a 6-line code which can even be a one liner

It looks like it actually is a one-liner, just spaced out for clarity. 😆

Do we really need to add an npm dependency for this?

Nope, we could totally just copy-paste this into our own middleware. However, does it really matter when we're bundling/minifying it anyway? I think the main benefit of the npm dependency is just for clarity.

I'm not very keen on an action creating multiple actions almost in a cascading manner.

I don't feel it's in a cascading manner. There are several situations in an app like WooCommerce where you need to take a single set of data as input and fire off multiple asynchronous API calls. This is what we're anticipating here. It makes sense to handle them individually but at the same time, and not as a unit (like in a thunk/promise) as that's how the API will handle them on the other side.

[coderkevin]

Not sure I like it, but a thunk could very easily do something like this:

I thought we were trying to get away from thunks.

[umurkontaci]

Nope, we could totally just copy-paste this into our own middleware. However, does it really matter when we're bundling/minifying it anyway? I think the main benefit of the npm dependency is just for clarity.

I would prefer not adding an npm dependency for one liners. Not because the shipped code would be different, but it's just another dependency that people have to install and we have to keep track of the versions (albeit very loosely).

I don't feel it's in a cascading manner.

Your use case might not be in a cascading manner, but this is a middleware that will enable dispatching actions which will dispatch more actions, which can even dispatch more actions. I don't think it's the right direction in general. I feel like it can create pathway to hard-to-catch bugs.

There are several situations in an app like WooCommerce where you need to take a single set of data as input and fire off multiple asynchronous API calls.

Could you give an example and why any of the approaches @aduth suggested won't work?

[coderkevin]

Could you give an example and why any of the approaches @aduth suggested won't work?

@aduth's approach would work. However it is depending on redux-thunk, which to my knowledge is discouraged in Calypso these days. If we're good with adding a thunk here, then it will work just fine.

[aduth]

Not sure I like it, but a thunk could very easily do something like this:

I thought we were trying to get away from thunks.

Hence why I'm not sure I like it 😛

Seems to come down to weighing the overhead cost of introducing another supported dispatch flow against using the hammer that is thunk (at the benefit that it's already present and familiar).

[coderkevin]

Seems to come down to weighing the overhead cost of introducing another supported dispatch flow against using the hammer that is thunk (at the benefit that it's already present and familiar).

I wonder if this should eventually work into more of a data-layer flow at some point. I'm working on getting that supported in extensions, so maybe we should just use redux-thunk with the intentions of moving it to a more robust data-layer approach after that's available.

[blowery]

I'm on the "just dispatch more than one action" side of the fence here. It's easy to understand, direct, and doesn't require having a middleware.

[coderkevin]

@blowery I think we all agree that more than one action needs to be dispatched, but the question is: how? I think we can all agree that this kind of business logic doesn't belong in the component, and the component is where action creators are bound to the redux dispatch object. So it's only in the component where we have direct access to the dispatcher to dispatch more than one action. The only other way around this is to use middleware. Either redux-thunk by dispatching a function that then dispatches more actions, or redux-multi which allows the dispatching of an array of actions.

So it seems to me we only have two choices: Business logic in the component, or use middleware.

Edit: There is a third option, and that is passing off the redux dispatch object to a helper function, where the business logic would live, but I don't feel really good about passing the dispatcher around in this way.

[blowery]

I think we all agree that more than one action needs to be dispatched, but the question is: how? I think we can all agree that this kind of business logic doesn't belong in the component, and the component is where action creators are bound to the redux dispatch object.

Can you be more concrete here? What kind of logic? Why doesn't the logic belong in a component? I dug through #12684 quickly and it wasn't clear to me where this would fit in. How does having an action mapper like this help?

[coderkevin]

Why doesn't the logic belong in a component?

From what I understand, we're all trying to keep business logic out of the components so we can get a better separation between application logic code and the view layer.

In this instance, it's where a single user action triggers multiple API actions. Where would you recommend putting that logic, @blowery ?

[blowery]

How would the code change if we had this middleware in place? Can you show an example?

[blowery]

In this instance, it's where a single user action triggers multiple API actions. Where would you recommend putting that logic, @blowery ?

function MyComponent( { actionOne, actionTwo, actionThree } ) {
  function handleAction() {
    actionOne();
    actionTwo();
    actionThree();
  }
  return <button onClick={ handleAction }>Do a Thing</button>;
}

export default connect( ... )( MyComponent );

[blowery]

i'm assuming the alternative this enables is

// action.js
export default function() {
  return [
    actionOne(),
    actionTwo(),
    actionThree(),
  ];
}

// component.js
function MyComponent( { actionOne, actionTwo, actionThree } ) {
  function handleAction() {
    action();
  }
  return <button onClick={ handleAction }>Do a Thing</button>;
}

export default connect( ... )( MyComponent );

[coderkevin]

While the multiple action firing is part of this, the other part is mapping the single user action to these multiple calls of the same action. It's a considerable amount of code that we'd rather not include in the standard event handling functions of a component. See the example here:

	updateVariations() {
		const { props } = this;
		const newVariations = this.generateVariations( props.product.variationTypes, props.product.variations || [] );
		newVariations.map( function( newVariation ) {
			props.addVariation( null, newVariation );
		} );
	}

	render() {
		...
		return <button onClick={ this.updateVariations }>Update</button>
	}

Note that generateVariations() isn't included in this snippet. Suffice to say it's a decent amount of code that is needed to compile state data into something the API is expecting. It's not directly tied to UI and it's lengthy, which IMO is good reason for not including this code in the UI event handling directly.

We're definitely open to different approaches here. Personally, the only things I feel strongly about are:

1. This logic should be centralized in one place (one file)
2. It shouldn't be in the same file as HTML markup.

Given that, I've been thinking of another approach where code like this is contained in a parent component given for just this purpose. Such a component could use react-redux connect() to get the needed action creators, and then could provide a function that takes in what's needed (variationTypes and variations in this case), to do the work to generate those actions. I'd be okay with this, or any another approach that gets this code one layer below the view.

[blowery]

Awesome, thanks for that! That helps clear up the motivation.

Kind of a tangent, but have you considered adding a addVariations action that takes > 1 new variation? As you have it now, you're going to generate a potential change event (assuming the new variation is in fact new) for each variation, which is going to try to run the connect mappers for whatever's currently connected. By consolidating everything down to one action, you'll only fire one change.

For the generator, it doesn't appear that it uses any component state, everything is passed in, so yeah, that would be a great candidate for extraction, maybe to a lib/ style thing.