Skip to content
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.

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

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

Merged
merged 12 commits into from Apr 12, 2019

Conversation

Projects
None yet
3 participants
@nerrad
Copy link
Contributor

commented Apr 4, 2019

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.

@nerrad nerrad self-assigned this Apr 4, 2019

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

@nerrad nerrad marked this pull request as ready for review Apr 4, 2019

@nerrad nerrad requested review from aduth and youknowriad as code owners Apr 4, 2019

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

This comment has been minimized.

Copy link
@aduth

aduth Apr 4, 2019

Member

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

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 4, 2019

Author Contributor

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).

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

This comment has been minimized.

Copy link
@aduth

aduth Apr 4, 2019

Member

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

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 4, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@aduth

aduth Apr 5, 2019

Member

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

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 6, 2019

Author Contributor

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

  • *withPromise() { for functions in an object property shorthand
  • function* getFoo for declared functions.
@@ -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.

This comment has been minimized.

Copy link
@aduth

aduth Apr 5, 2019

Member

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?

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 6, 2019

Author Contributor

True, I clarified in 78185ab

@@ -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`.

This comment has been minimized.

Copy link
@aduth

aduth Apr 5, 2019

Member

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() );

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 5, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@aduth

aduth Apr 8, 2019

Member

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).

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 8, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Member

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?

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.

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 11, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 11, 2019

Author Contributor

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 added some commits Apr 4, 2019

@nerrad nerrad force-pushed the BUG/restore-action-generator-return branch from 78185ab to 2e7e5ba Apr 11, 2019

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

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

aduth approved these changes Apr 11, 2019

Copy link
Member

left a comment

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.

@@ -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

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Member

Needs to be updated given latest revisions?

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 11, 2019

Author Contributor

yup, forgot I manually added this.

nerrad added some commits Apr 11, 2019

@aduth

This comment has been minimized.

Copy link
Member

commented on 0bb2bf7 Apr 11, 2019

I struggled when considering to leave the review comment on the generated documentation or on the source. In retrospect, I probably should have just duplicated the comment to both. 😄

@nerrad nerrad merged commit 68fb39e into master Apr 12, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the BUG/restore-action-generator-return branch Apr 12, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.