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.

Sign up for GitHub

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
Merged

Data: Return result of middleware chain in resolvers cache middleware #14711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23f09ac
Data: Return result of middleware chain in resolvers cache middleware
aduth Mar 29, 2019
8ebd1b3
Data: Avoid returning result of store.dispatch in public interface
aduth Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/data/src/namespace-store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@aduth aduth Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Line 260 in b3961d0

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

Copy link
Contributor

@nerrad nerrad Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nerrad nerrad Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const createBoundAction = ( action ) => ( ...args ) => {
store.dispatch( action( ...args ) );
};

return mapValues( actions, createBoundAction );
}

Expand Down
41 changes: 41 additions & 0 deletions packages/data/src/namespace-store/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,45 @@ describe( 'controls', () => {
expect( action1 ).toBeCalled();
} );
} );

it( 'resolves in expected order', ( done ) => {
const actions = {
wait: () => ( { type: 'WAIT' } ),
receive: ( items ) => ( { type: 'RECEIVE', items } ),
};

registry.registerStore( 'store', {
reducer: ( state = null, action ) => {
if ( action.type === 'RECEIVE' ) {
return action.items;
}

return state;
},
selectors: {
getItems: ( state ) => state,
},
resolvers: {
* getItems() {
yield actions.wait();
yield actions.receive( [ 1, 2, 3 ] );
},
},
controls: {
WAIT() {
return new Promise( ( resolve ) => process.nextTick( resolve ) );
},
},
} );

registry.subscribe( () => {
const isFinished = registry.select( 'store' ).hasFinishedResolution( 'getItems' );
if ( isFinished ) {
expect( registry.select( 'store' ).getItems() ).toEqual( [ 1, 2, 3 ] );
done();
}
} );

registry.select( 'store' ).getItems();
} );
} );
2 changes: 1 addition & 1 deletion packages/data/src/resolvers-cache-middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const createResolversCacheMiddleware = ( registry, reducerKey ) => () => ( next
registry.dispatch( 'core/data' ).invalidateResolution( reducerKey, selectorName, args );
} );
} );
next( action );
return next( action );
};

export default createResolversCacheMiddleware;
3 changes: 2 additions & 1 deletion packages/data/src/test/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,8 @@ describe( 'createRegistry', () => {
},
} );

registry.dispatch( 'counter' ).increment(); // state = 1
const dispatchResult = registry.dispatch( 'counter' ).increment(); // state = 1
expect( dispatchResult ).toBe( undefined ); // Actions are implementation detail.
registry.dispatch( 'counter' ).increment( 4 ); // state = 5
expect( store.getState() ).toBe( 5 );
} );
Expand Down