Data: Restore dispatch actions returning actions that are a Promise #14830

nerrad · 2019-04-04T20:34:17Z

Description

This pull is prompted by some discussion beginning here around what dispatch actions return. Prior to this pull dispatching a generator action would return a promise and after that pull it returned undefined. While #14711 fixed a related issue with returning the result of middleware chains, it removed the behaviour that existed prior to #14634 for dispatch action returns.

In a plugin I'm developing, we came to rely on that behaviour because it was useful for complex dispatch chains via action-generators where the response of one dispatch could inform the shape of the next dispatch. So I was able to have something like this as a control which allowed for awaiting the result of the action generator promise:

async RESOLVE_DISPATCH( { reducerKey, dispatchName, args } ) {
		return await dispatchData( reducerKey )[ dispatchName ]( ...args );
	},

This pull seeks to be more specific on what wp.data.dispatch( 'storeName' ).action() can return and only return if the action is a promise. This also adds appropriate tests.

How has this been tested?

Verify this fixes issues I was seeing in third party work I'm doing.
Verify existing/new unit tests pass.
Verify there is no breakage in e2e tests.

Types of changes

This is a non-breaking change (and in fact restores regression incurred that was breaking).

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

aduth · 2019-04-04T20:43:02Z

packages/data/src/namespace-store/test/index.js

+			reducer: () => {},
+			actions,
+		} );
+		expect( isPromise( registry.dispatch( 'store' ).withPromise() ) )


Per #14711 (comment), we should have an assertion for the expected resolved value of the promise as well.

ya good call. I took this as incentive to expand the tests to cover most return scenarios (including ones similar to the use-case I have in a plugin I'm writing). I think the test coverage better describes the expected behaviour now (which of course may still be up for discussion if desired).

aduth · 2019-04-04T20:44:58Z

packages/data/src/namespace-store/index.js

-		store.dispatch( action( ...args ) );
+		const result = store.dispatch( action( ...args ) );
+		if ( isPromise( result ) ) {
+			return result;


Per #14711 (comment) , we should document this return value somewhere. Auto-doc probably won't help us. Probably along the mention of:

store.dispatch( action: Object ): Given an action object, calls the registered reducer and updates the state value.

https://github.com/WordPress/gutenberg/blob/master/packages/data/README.md

I added a note in the js doc block for the publicly exposed dispatch api (see 93172cf) as opposed to what you referenced here which is the internal redux dispatch on the returned store object. I think this is likely what you meant. I'm struggling a bit with the wording for additional docs so help is appreciated but imo I think we should keep it fairly brief for now.

aduth · 2019-04-05T20:48:46Z

packages/data/src/namespace-store/test/index.js

+	describe( 'various action types have expected response and resolve as ' +
+		'expected', () => {
+		const actions = {
+			*withPromise() {


Aside: We should devise a spacing convention for *, as it's not enforced one way or the other currently, and I've personally written code with the space between the * and the function name.

Not evident here, the guideline should include whether a space should occur prior the * as in function* getFoo or function * getFoo

My preference if we enforce some convention would be to do:

*withPromise() { for functions in an object property shorthand

function* getFoo for declared functions.

Related: The finishing touches are being put on wp-pretter@2 which will answer all your function * () {} questions 😁

aduth · 2019-04-05T20:50:02Z

packages/data/src/index.js

@@ -79,6 +79,9 @@ export const select = defaultRegistry.select;
 * Given the name of a registered store, returns an object of the store's action creators.
 * Calling an action creator will cause it to be dispatched, updating the state value accordingly.
 *
+ * Note: If the action creator is a generator then a promise is returned.


It's not necessarily limited to generators, is it? From the fact we're using just a simple promise middleware, I'd imagine any promise action could be supported?

True, I clarified in 78185ab

aduth · 2019-04-05T20:54:33Z

packages/data/src/index.js

@@ -79,6 +79,9 @@ export const select = defaultRegistry.select;
 * Given the name of a registered store, returns an object of the store's action creators.
 * Calling an action creator will cause it to be dispatched, updating the state value accordingly.
 *
+ * Note: If the action creator is a generator then a promise is returned.
+ * Otherwise, the dispatch will return `undefined`.


I'm starting to wonder if we'd just be better off implementing createBoundAction as:

return Promise.resolve( store.dispatch( action( ...args ) ) );

Which automatically normalizes everything as a promise, avoiding any confusion about what's returned and under what circumstances.

A possible further step to make a stronger guarantee of resolving-to-undefined:

return Promise.resolve( store.dispatch( action( ...args ) ) ).then( () => Promise.resolve() );

Re: modifying createBoundAction to always return as a promise.
Ya that would help with there being a consistent returned type.

Re: always resolving to undefined. This I'm not sure about as it would break our implementation. Is the concern mostly about potentially returning action objects?

Note, without the redux-routine middleware applied, I observed in a test (that I didn't include here) that if a promise is returned from the action object and resolves to a non action like object, an error is thrown Edit: I thought I had observed this but I decided to add back in a test for this condition and discovered that it didn't behave as described here. So, the behaviour for a returned promise is consistent currently.

I'm curious, what's the reasoning behind wanting to always resolve to undefined as suggested? Is it mostly because of convention (or its a bad pattern otherwise)? I'm asking out of concern there's something I'm ignorant on.

I'm curious, what's the reasoning behind wanting to always resolve to undefined as suggested?

For now, I see it as part of being more explicit about exactly what we're expecting. Because otherwise, what do we expect it to resolve to? I'm not sure it's a question we've sought to answer yet, and have deferred largely to things like action objects and raw reducer state to be internal implementation details. At least for core code, this has the benefit of reducing the maintenance burden of backwards-compatibility (anticipating and allowing for changes in reducer state, presumably also action object shape).

For now, I see it as part of being more explicit about exactly what we're expecting. Because otherwise, what do we expect it to resolve to? I'm not sure it's a question we've sought to answer yet, and have deferred largely to things like action objects and raw reducer state to be internal implementation details. At least for core code, this has the benefit of reducing the maintenance burden of backwards-compatibility (anticipating and allowing for changes in reducer state, presumably also action object shape).

Gotcha 👍 . Could we say we're being explicit enough by indicating you can always expect a promise? I think it's a reasonable expectation that action like objects will always resolve to undefined but if implementors of the data api need to resolve to some other value for their own implementation that's up to them? So the api is still fairly explicit imo:

non generator actions must always return an action object.

actions can return a promise that could resolve to anything.

generator actions yield actions and can return anything (which will be exposed via a Promise that resolves on generator completion).

I think it's reasonable for consuming code to understand that promises returned by dispatch actions may resolve to undefined, so only handle the resolved value if certain the dispatched action resolves to one.

Maintenance wise, I think it's reasonable to simply always return a promise on dispatched actions and that's the contract established. It gives the flexibility for those depending on the wp.data api to utilize the dispatched actions as in the use-case I have.

I'm generally leaning to being okay with all of this, at least in the sense that by most default usage, you'd expect undefined, except when going out of your way to do otherwise. And by the fact that this restores a behavior existing prior to Gutenberg 5.4.

A possible issue is that "going out of your way" is not really as hard as it seems. For example, what would the promise of this existing action creator resolve to?

gutenberg/packages/block-editor/src/store/actions.js

Lines 382 to 411 in 7dfc268

export function* insertBlocks(

blocks,

index,

rootClientId,

updateSelection = true

) {

blocks = castArray( blocks );

const allowedBlocks = [];

for ( const block of blocks ) {

const isValid = yield select(

'core/block-editor',

'canInsertBlockType',

block.name,

rootClientId

);

if ( isValid ) {

allowedBlocks.push( block );

}

}

if ( allowedBlocks.length ) {

return {

type: 'INSERT_BLOCKS',

blocks: allowedBlocks,

index,

rootClientId,

time: Date.now(),

updateSelection,

};

}

}

I'm expecting it resolves to the action object.

Also as noted in Slack DM, we discussed that it may not be entirely obvious that if the promise resolves to a value which has a type property, whether that's an action or not, it would be passed through to dispatch.

For example, what would the promise of this existing action creator resolve to?

Good question, I think it'd be good if tests in this branch cover that kind of scenario. I'll update the tests to cover that.

Good question, I think it'd be good if tests in this branch cover that kind of scenario. I'll update the tests to cover that.

Turns out they already do. As you suspected an action object is returned.

nerrad · 2019-04-11T19:17:46Z

In the latest changes after rebase:

dispatched actions now always return a promise.
this means that action objects are no longer a hidden implementation detail (action like objects could be the value resolved to).
updates test expectations due to the changes.

I think this is an acceptable resolution because returning a promise clearly communicates that dispatches are not synchronous. Developers using the returned promise would therefore be required to correctly handle it.

The use-case this solves is with nested action-generators where the result of a dispatch might be needed to inform the contents of a subsequent dispatch. Example: A store state that manages non-persisted relationships between entity in the state and needs to update those relationship maps after an entity is is persisted to the server via apiFetch (which returns a new id for the entity to replace the temporary id created client side).

aduth

While I don't know that we'd ever want to leverage the resolved value in this project, I also don't think we need hide it nor deal with complexities of sniffing the action to make some distinct treatment of the resolved value depending on whether it's action-like. I have trouble foreseeing a risk in "exposing" the shape of these actions, and while we don't otherwise really care to align 1-to-1 with Redux equivalents, it should be noted that Redux's Store#dispatch has a like return value.

The use-case this solves is with nested action-generators where the result of a dispatch might be needed to inform the contents of a subsequent dispatch.

I acknowledge the need here and I don't have an alternative to offer, but worry that this does blur the line of treating results from actions creators as actions vs. whatever it is you're using as an informing value.

aduth · 2019-04-11T19:37:23Z

packages/data/README.md

@@ -332,6 +332,9 @@ _Returns_
 Given the name of a registered store, returns an object of the store's action creators.
 Calling an action creator will cause it to be dispatched, updating the state value accordingly.

+Note: If the action creator is a generator or returns a promise, a promise is


Needs to be updated given latest revisions?

yup, forgot I manually added this.

…ordPress#14830) All dispatched action creators will now return a Promise.

nerrad self-assigned this Apr 4, 2019

nerrad added [Package] Data /packages/data [Type] Bug An existing feature does not function as intended labels Apr 4, 2019

nerrad added this to the 5.5 (Gutenberg) milestone Apr 4, 2019

nerrad marked this pull request as ready for review April 4, 2019 20:35

nerrad requested review from aduth and youknowriad as code owners April 4, 2019 20:35

aduth reviewed Apr 4, 2019

View reviewed changes

aduth reviewed Apr 5, 2019

View reviewed changes

nerrad added 10 commits April 11, 2019 11:22

restore dispatch actions returning actions that are a Promise

a7aa52e

expand tests to cover all possible dispatch action responses

4c81ff9

expand jsdoc for wp.data.dispatch calls

53e61f6

Add changelog entry

0de7797

update docs

fbc7e7f

more test coverage (without controls)

31c171b

update docs

3861ca7

Always return a promise on dispatch

9c9943b

update test expectations

9675f41

update changelog

2e7e5ba

nerrad force-pushed the BUG/restore-action-generator-return branch from 78185ab to 2e7e5ba Compare April 11, 2019 19:14

aduth approved these changes Apr 11, 2019

View reviewed changes

nerrad added 2 commits April 11, 2019 15:52

Update doc.

4574a38

woops updated docs the wrong way!

0bb2bf7

nerrad merged commit 68fb39e into master Apr 12, 2019

nerrad added [Type] Enhancement A suggestion for improvement. and removed [Type] Bug An existing feature does not function as intended labels Apr 12, 2019

aduth deleted the BUG/restore-action-generator-return branch April 12, 2019 19:35

mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019

Data: Restore dispatch actions returning actions that are a Promise (W…

65ae4a3

…ordPress#14830) All dispatched action creators will now return a Promise.

youknowriad added the [Type] Bug An existing feature does not function as intended label Apr 15, 2019

This was referenced Apr 15, 2019

Bump version number to 5.5.0-rc.1 #14981

Merged

Bump/5.5 #15011

Closed

Release/5.5 #15012

Closed

Bump plugin version to 5.5 #15013

Merged

nerrad mentioned this pull request Nov 19, 2019

Extra action triggered when saving posts #17485

Closed

nerrad mentioned this pull request Mar 2, 2020

Create Context Provider for Notices w/ Notices API woocommerce/woocommerce-blocks#1843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data: Restore dispatch actions returning actions that are a Promise #14830

Data: Restore dispatch actions returning actions that are a Promise #14830

nerrad commented Apr 4, 2019 •

edited

Loading

aduth Apr 4, 2019

nerrad Apr 4, 2019

aduth Apr 4, 2019

nerrad Apr 4, 2019

aduth Apr 5, 2019

nerrad Apr 6, 2019

sirreal Apr 22, 2020

aduth Apr 5, 2019

nerrad Apr 6, 2019

aduth Apr 5, 2019

nerrad Apr 5, 2019 •

edited

Loading

aduth Apr 8, 2019

nerrad Apr 8, 2019 •

edited

Loading

aduth Apr 11, 2019

nerrad Apr 11, 2019

nerrad Apr 11, 2019

nerrad commented Apr 11, 2019 •

edited

Loading

aduth left a comment

aduth Apr 11, 2019

nerrad Apr 11, 2019

	export function* insertBlocks(
	blocks,
	index,
	rootClientId,
	updateSelection = true
	) {
	blocks = castArray( blocks );
	const allowedBlocks = [];
	for ( const block of blocks ) {
	const isValid = yield select(
	'core/block-editor',
	'canInsertBlockType',
	block.name,
	rootClientId
	);
	if ( isValid ) {
	allowedBlocks.push( block );
	}
	}
	if ( allowedBlocks.length ) {
	return {
	type: 'INSERT_BLOCKS',
	blocks: allowedBlocks,
	index,
	rootClientId,
	time: Date.now(),
	updateSelection,
	};
	}
	}

Data: Restore dispatch actions returning actions that are a Promise #14830

Data: Restore dispatch actions returning actions that are a Promise #14830

Conversation

nerrad commented Apr 4, 2019 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

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

nerrad Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Apr 8, 2019 • 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

nerrad commented Apr 11, 2019 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad commented Apr 4, 2019 •

edited

Loading

nerrad Apr 5, 2019 •

edited

Loading

nerrad Apr 8, 2019 •

edited

Loading

nerrad commented Apr 11, 2019 •

edited

Loading