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: Return result of middleware chain in resolvers cache middleware #14711

Merged
merged 2 commits into from Apr 4, 2019

Conversation

Projects
None yet
3 participants
@aduth
Copy link
Member

commented Mar 29, 2019

Related: #14634 (comment)

This pull request seeks to resolve an issue with resolver behavior where resolution is marked as finished before the results of a generator control have finished. This is due to the fact that we await the result of store.dispatch as if it would return an action, but the order of the middleware chain is such that the promise resolvers cache middleware runs first, and it previously would always return undefined (ignoring the fact that the result of next was the promise returned by redux-routine), causing the resolution to be marked as finished immediately.

await store.dispatch( action );

return ( action ) => new Promise( ( resolve, reject ) =>

Testing instructions:

Verify that unit tests pass:

npm run test-unit

As I was only able to reproduce in a local working branch, I am not certain steps to verify the behavior in the application. As an alternative, it should be sufficient to verify that other resolved behaviors work as expected (e.g. saving a post).

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

The previous bug was hiding another bug, which was that we would return the result of store.dispatch in the public API (i.e. the action dispatched). This is fixed in 8ebd1b3.

As best I can tell, this was never expected, and never occurred, but only because of how the resolvers cache middleware would drop the result.

I suppose an additional testing step is to verify both in this branch and a stable WordPress release that entering the following in your console produces a value of undefined:

wp.data.dispatch( 'core/editor' ).editPost( { title: 'Hello world' } );
@nerrad

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

The previous bug was hiding another bug, which was that we would return the result of store.dispatch in the public API (i.e. the action dispatched). This is fixed in 8ebd1b3.

So currently, if an action is a generator action, a promise is returned. Currently, in a plugin I write I take advantage of that behaviour to not only await the completion of the dispatch but also return a value from the action generator useful for chaining additional dispatches. With this change will this behaviour break?

@aduth

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

So currently, if an action is a generator action, a promise is returned. Currently, in a plugin I write I take advantage of that behaviour to not only await the completion of the dispatch but also return a value from the action generator useful for chaining additional dispatches. With this change will this behaviour break?

As far as I'm aware, it was never an intended behavior to support. In fact, tests would previously forbid this, or at least for certain types of actions from the withDispatch component.

That's unfortunate to hear there's some potential compatibility issue here. It'll require some more thought, then.

@nerrad

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

This is in unreleased code so at least for me, this is something I can probably work out alternatives for if this was unintended. There is some concern there may be others relying on the same behaviour though.

@@ -187,7 +187,10 @@ function mapSelectors( selectors, store, registry ) {
* @return {Object} Actions mapped to the redux store provided.
*/
function mapActions( actions, store ) {
const createBoundAction = ( action ) => ( ...args ) => store.dispatch( action( ...args ) );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 29, 2019

Contributor

For me this was an intended behavior though. It was explicitely meant to address the issue solved by the PR.

This comment has been minimized.

Copy link
@aduth

aduth Apr 2, 2019

Author Member

For me this was an intended behavior though. It was explicitely meant to address the issue solved by the PR.

What is the intended behavior? I need to check again, but as I understand the bug to be is that nothing will be returned by promise middleware (the first in the chain) in the current master branch. The change on this line seeks only to preserve that. Otherwise, what is the expected return value when dispatching an action?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 3, 2019

Contributor

I think the expected return value here is a promise that is resolved once all the action flow (controls, sync actions...) is finished.

This comment has been minimized.

Copy link
@aduth

aduth Apr 3, 2019

Author Member

I'm wondering if we're getting mixed up on the expectations of a dispatch which occurs via the result of the public interface wp.data.dispatch, and that of the internal store.dispatch, where the latter does in-fact (and still, after these changes) return a promise.

await store.dispatch( action );

I tested again on master, and neither a synchronous nor control action returns anything through wp.data.dispatch:

wp.data.dispatch( 'core/editor' ).editPost( { title: 'hello' } );
// undefined
wp.data.dispatch( 'core/editor' ).savePost();
// undefined

However, it was the case that generator actions would return promises as of v5.3.0 .

wp.data.dispatch( 'core/editor' ).editPost( { title: 'hello' } );
// undefined
wp.data.dispatch( 'core/editor' ).savePost();
// Promise {<pending>}

This aligns with @nerrad 's earlier comment. I suspect it changed as a result of #14634.

So, the question(s) then are: Was it intentional to return a Promise for wp.data.dispatch and not just the internal store.dispatch? If so, shall we restore that behavior? And should it return a Promise for the synchronous editPost as above, or would it be fine enough since it would be normalized to a resolved promise in the context of await or a then return value (but not directly .then-able itself without manually normalizing via Promise.resolve).

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 3, 2019

Contributor

It has been beneficial in our ("our" meaning the team I work with implementing the data api, not "our" meaning the GB team) usage that action generators result in a returned promise as there are many use-cases where the result of a save/update could be utilized immediately in further dispatched actions specific to an implementation (as opposed to general via the action). So I'm not opposed to leaving it as. I do think having "regular" actions not returning anything is fine.

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 3, 2019

Contributor

I suspect it changed as a result of #14634.

wp.data.dispatch( 'somestore').someActionGenerator() has returned a pending promise for quite a while now. I don't think #14634 introduced it.

This comment has been minimized.

Copy link
@aduth

aduth Apr 3, 2019

Author Member

wp.data.dispatch( 'somestore').someActionGenerator() has returned a pending promise for quite a while now. I don't think #14634 introduced it.

I'm suggesting it did the opposite: It removed it.

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 3, 2019

Contributor

YIKES! It did! I haven't tested my work against the latest GB for a while. So things are broke :( (for us).

@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

All said and done, this pull request currently behaves identically to what exists today in master in the return value of wp.data.dispatch , and has the added benefit of fixing the issue described in #14634 (comment) .

It seems like there's some open question as to what dispatch should be expected to return, but I might suggest that it be explored separately.

@nerrad

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@aduth I'm good with this getting merged in. If I did a pull restoring the response of wp.data.dispatch( 'storeName' ).generatorAction as a promise, we could continue the discussion there for what should be done there? If yes, I probably get a pull done sometime tonight or tomorrow.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

If I did a pull restoring the response of wp.data.dispatch( 'storeName' ).generatorAction as a promise, we could continue the discussion there for what should be done there?

That sounds reasonable, sure.

Much of what confused me here is that we were never explicit in what behavior we expect from dispatch. We all seemed to have our own personal interpretations of what it should be and/or came to rely on the behavior as it happened to be implemented. For a pull request, we should document and test the expected behavior. Doing so I think will also help ensure consistency, so that it isn't a specific behavior for a particular kind of action (generator actions). It also occurs to me that we should be more explicit for cases where we expect no return value, rather than treating it as some assumed default. In my interpretation of the expected behavior, we should have had a test case which includes expect( result ).toBe( undefined );. This sort of existed in the tests for withDispatch, but only indirectly (though, to its credit, its failure was what prompted the discussion).

It's probably more appropriate to share my opinions on a pull request, but I also hope (a) introducing imperative patterns (following a promise resolution vs. subscribing for updates) doesn't contradict existing patterns and (b) we don't expose (resolve to) the actual action object(s), which ought continue to be treated as an implementation detail. Other than that, I don't really have much of an objection to the idea that dispatch could return a Promise which resolves (to undefined) once it's "complete".

@nerrad

nerrad approved these changes Apr 4, 2019

Copy link
Contributor

left a comment

let's get this in 👍

@aduth aduth merged commit 1f8f7d7 into master Apr 4, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the fix/data-await-registry-resolution branch Apr 4, 2019

@youknowriad youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019

This was referenced Apr 17, 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.