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
7 changes: 7 additions & 0 deletions packages/data/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## Master

### Bug Fix

- Restore functionality of action-generators returning a Promise. Clarify intent and behaviour for `wp.data.dispatch` behaviour. Dispatch actions now always
return a promise ([#14830](https://github.com/WordPress/gutenberg/pull/14830)

## 4.3.0 (2019-03-06)

### Enhancements
Expand Down
3 changes: 3 additions & 0 deletions packages/data/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: Action creators returned by the dispatch will return a promise when
they are called.

_Usage_

```js
Expand Down
21 changes: 16 additions & 5 deletions packages/data/src/components/with-dispatch/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ describe( 'withDispatch', () => {

return {
increment: () => {
const actionReturnedFromDispatch = _dispatch( 'counter' ).increment( count );
expect( actionReturnedFromDispatch ).toBe( undefined );
const actionReturnedFromDispatch = Promise.resolve( _dispatch( 'counter' ).increment( count ) );
expect( actionReturnedFromDispatch ).resolves.toEqual(
{
type: 'increment',
count,
}
);
},
};
} )( ( props ) => <button onClick={ props.increment } /> );
Expand Down Expand Up @@ -120,7 +125,8 @@ describe( 'withDispatch', () => {
expect( secondRegistryAction ).toHaveBeenCalledTimes( 2 );
} );

it( 'always calls select with the latest state in the handler passed to the component', () => {
it( 'always calls select with the latest state in the handler passed to ' +
'the component', () => {
const store = registry.registerStore( 'counter', {
reducer: ( state = 0, action ) => {
if ( action.type === 'update' ) {
Expand All @@ -142,8 +148,13 @@ describe( 'withDispatch', () => {
update: () => {
const innerCount = _select( 'counter' ).getCount();
expect( innerCount ).toBe( outerCount );
const actionReturnedFromDispatch = _dispatch( 'counter' ).update( innerCount + 1 );
expect( actionReturnedFromDispatch ).toBe( undefined );
const actionReturnedFromDispatch = Promise.resolve(
_dispatch( 'counter' ).update( innerCount + 1 )
);
expect( actionReturnedFromDispatch ).resolves.toEqual( {
type: 'update',
count: innerCount + 1,
} );
},
};
} )( ( props ) => <button onClick={ props.update } /> );
Expand Down
3 changes: 3 additions & 0 deletions packages/data/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: Action creators returned by the dispatch will return a promise when
* they are called.
*
* @param {string} name Store name
*
* @example
Expand Down
2 changes: 1 addition & 1 deletion packages/data/src/namespace-store/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ function mapSelectors( selectors, store, registry ) {
*/
function mapActions( actions, store ) {
const createBoundAction = ( action ) => ( ...args ) => {
store.dispatch( action( ...args ) );
return Promise.resolve( store.dispatch( action( ...args ) ) );
};

return mapValues( actions, createBoundAction );
Expand Down
93 changes: 93 additions & 0 deletions packages/data/src/namespace-store/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,97 @@ describe( 'controls', () => {

registry.select( 'store' ).getItems();
} );
describe( 'various action types have expected response and resolve as ' +
'expected with controls middleware', () => {
const actions = {
*withPromise() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  • *withPromise() { for functions in an object property shorthand
  • function* getFoo for declared functions.

Copy link
Member

Choose a reason for hiding this comment

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

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

yield { type: 'SOME_ACTION' };
return yield { type: 'TEST_PROMISE' };
},
*withNormal() {
yield { type: 'SOME_ACTION' };
yield { type: 'SOME_OTHER_ACTION' };
},
*withNonActionLikeValue() {
yield { type: 'SOME_ACTION' };
return 10;
},
normalShouldFail: () => 10,
normal: () => ( { type: 'NORMAL' } ),
};
beforeEach( () => {
registry.registerStore( 'store', {
reducer: () => {},
controls: {
TEST_PROMISE() {
return new Promise( ( resolve ) => resolve( 10 ) );
},
},
actions,
} );
} );
it( 'action generator returning a yielded promise control descriptor ' +
'resolves as expected', async () => {
const withPromise = registry.dispatch( 'store' ).withPromise();
await expect( withPromise ).resolves.toEqual( 10 );
} );
it( 'action generator yielding normal action objects resolves as ' +
'expected', async () => {
const withNormal = registry.dispatch( 'store' ).withNormal();
await expect( withNormal ).resolves.toBeUndefined();
} );
it( 'action generator returning a non action like value', async () => {
const withNonActionLikeValue = registry.dispatch( 'store' )
.withNonActionLikeValue();
await expect( withNonActionLikeValue ).resolves.toEqual( 10 );
} );
it( 'normal dispatch action throwing error because no action ' +
'returned', () => {
const testDispatch = () => registry.dispatch( 'store' ).normalShouldFail();
expect( testDispatch ).toThrow(
'Actions must be plain objects. Use custom middleware for async actions.'
);
} );
it( 'returns action object for normal dispatch action', async () => {
await expect( registry.dispatch( 'store' ).normal() )
.resolves
.toEqual( { type: 'NORMAL' } );
} );
} );
describe( 'action type resolves as expected with just promise ' +
'middleware', () => {
const actions = {
normal: () => ( { type: 'NORMAL' } ),
withPromiseAndAction: () => new Promise(
( resolve ) => resolve( { type: 'WITH_PROMISE' } )
),
withPromiseAndNonAction: () => new Promise(
( resolve ) => resolve( 10 )
),
};
beforeEach( () => {
registry.registerStore( 'store', {
reducer: () => {},
actions,
} );
} );
it( 'normal action returns action object', async () => {
await expect( registry.dispatch( 'store' ).normal() )
.resolves
.toEqual( { type: 'NORMAL' } );
} );
it( 'action with promise resolving to action returning ' +
'action object', async () => {
await expect( registry.dispatch( 'store' ).withPromiseAndAction() )
.resolves
.toEqual( { type: 'WITH_PROMISE' } );
} );
it( 'action with promise returning non action throws error', async () => {
const dispatchedAction = registry.dispatch( 'store' )
.withPromiseAndNonAction();
await expect( dispatchedAction ).rejects.toThrow(
'Actions must be plain objects. Use custom middleware for async actions.'
);
} );
} );
} );
13 changes: 9 additions & 4 deletions packages/data/src/test/registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ describe( 'createRegistry', () => {
} );

describe( 'dispatch', () => {
it( 'registers actions to the public API', () => {
it( 'registers actions to the public API', async () => {
const increment = ( count = 1 ) => ( { type: 'increment', count } );
const store = registry.registerStore( 'counter', {
reducer: ( state = 0, action ) => {
Expand All @@ -554,9 +554,14 @@ describe( 'createRegistry', () => {
increment,
},
} );

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