Promisify action creator return type for WP data dispatch #52530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noahtallen
added
[Type] Enhancement
noahtallen
commented
Jul 11, 2023
| @@ -6,7 +6,7 @@ import type { combineReducers as reduxCombineReducers } from 'redux'; | |||
|
|
|||
| type MapOf< T > = { [ name: string ]: T }; | |||
|
|
|||
| export type ActionCreator = Function | Generator; | |||
| export type ActionCreator = ( ...args: any[] ) => any | Generator; | |||
There was a problem hiding this comment.
Function doesn't let you add return types and isn't compatible with the arrow syntax, so it needs to be changed so that we can wrap around the type below.
|
Flaky tests detected in 5b7a9bb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5548325503
|
jsnajdr
reviewed
Jul 12, 2023
tyxla
reviewed
Jul 12, 2023
dmsnell
approved these changes
Jul 12, 2023
There was a problem hiding this comment.
If this matches the actual code let's do it. Thanks @noahtallen.
A couple notes though:
- @jsnajdr and @tyxla are right: use of the singular
PromisifiedActionCreatorwill help not only with code reuse, but TypeScript will give up in some anonymous cases like this where having a separated named type-level function lets it continue. I don't understand the heuristic it uses, but I've confirmed it in several different cases. - Let's ensure that we don't inadvertently trigger a host of "didn't wait for
Promiseto resolve" errors where code dispatches and moves on.
the more concerning thing to me is that we await the result of dispatch, but I don't suppose that has anything to do with matching the types to the reality.
|
Here is the key line in the code gutenberg/packages/data/src/redux-store/index.js Lines 200 to 203 in c355d54 I think redux itself is synchronous here, but we wrap |
noahtallen
commented
Jul 12, 2023
I linked the package locally to another repo, and didn't see this error (and I don't see it in Gutenberg either), so I think we're good! I verified the types as well: 
The action itself: 
|
|
Size Change: -68 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
The It's actually a bad idea to do this, because it's quite common that action dispatches are completely synchronous. And yet even these sync dispatches return a meaningless By the way, |
noahtallen
force-pushed
the
fix-wp-data-promise-return-type
branch
from
f008167 to
f3667d6
Compare
|
I spent some time testing this in a separate repo which has more Typescript stores. I improved the return type to correctly handle thunks (and async thunks). When the action creator's return type is another function, we then get the return type of the inner function, and use I created a few simple dummy actions and thunks to test the types with various combinations of arguments and async scenarios: // Action creators with and without actions:
const testAction = ( foo: string ) => ( { type: 'FOO', foo } );
const testAction2 = () => ( { type: 'BAR' } );
// Various thunks with combos of async and arguments:
const testThunk =
() =>
( { dispatch } ) => {
return 1234;
};
const testThunk1 = ( test: string ) => (): CheckoutPaymentMethodSlug => {
return 1 > 2 ? 'alipay' : 'apple-pay';
};
const testThunk2 = ( test: number ) => async () => {
return Promise.resolve( 'Hello world' );
};
const testThunk3 = ( foo: number, bar: string ) => async ( { dispatch } ) => {
return foo + bar;
};
// Elsewhere:
const {
testAction,
testAction2,
testThunk,
testThunk1,
testThunk2,
testThunk3,
} = useDispatch( testStore );
// typeof value is { type: string, foo: string }
testAction( 'hi' ).then( ( value ) => {} );
// typeof value is { type: string }
testAction2().then( ( value ) => {} );
// typeof value is number
testThunk().then( ( value ) => {} );
// typeof value is CheckoutPaymentMethodSlug
testThunk1( 'hi' ).then( ( value ) => {} );
// typeof value is string (importantly, NOT Promise<string>)
testThunk2( 1 ).then( ( value ) => {} );
// typeof value is string
testThunk3( 1, 'hello' ).then( ( value ) => {} ); |
noahtallen
commented
Jul 13, 2023
| @@ -28,8 +23,10 @@ import defaultRegistry from './default-registry'; | |||
| * ``` | |||
| * @return Object containing the action creators. | |||
| */ | |||
| export function dispatch< T extends StoreDescriptor< AnyConfig > >( | |||
| storeNameOrDescriptor: string | T | |||
| ): ActionCreatorsOf< ConfigOf< T > > { | |||
There was a problem hiding this comment.
While testing in another repo, I came across an error where dispatch( 'store-name' ) had the return type ActionCreatorsOf.... This return type doesn't work, because we aren't using the object format of the store name. useDispatch handles this correctly (the return type should be unknown or similar, since we don't know any metadata about the store), so this basically makes it more similar to those types.
|
Would appreciate a second review if folks have time, since I made some changes to account for thunks. |
tyxla
approved these changes
Jul 20, 2023
noahtallen
force-pushed
the
fix-wp-data-promise-return-type
branch
from
5b7a9bb to
8cbdc88
Compare
westonruter
added a commit
that referenced
this pull request
Jul 20, 2023
…ding-strategy * origin/trunk: (59 commits) Promisify action creator return type for WP data dispatch (#52530) [RNMobile] Add WP hook for registering non-core blocks (#52791) removes check for active preview device type to enable the fixed toolbar preference (#52770) Enforce checks against redeclaration for functions and classes (#52696) update appearance tools, (#52785) Behaviors: Extend Global Styles API to read/write behaviors config. (#52370) HeaderToolbar - Update inserterMethod meta data (#52735) add options for debugging php unit tests (#52778) Docs: Interactivity API > Getting Started Guide - minor adjustments (#52786) Footnotes: Use static closures when not using '' (#52781) Improve slug generation & matching in request utils (#52414) Open "docs" folder for the Interactivity API package and Getting Started Guide (#52462) Global Styles: Don't use named arguments for 'sprintf' (#52782) E2E utils - Update locator to hide the keyboard on iOS to pick the first element, on iPad two buttons are available and the second one makes the floating keyboard to show up (#52771) Patterns: Reinstate template parts mode spec (#52780) chore(release): publish Update changelog files Patterns: Fix empty general template parts category (#52747) Add id to pattern inserted notice to stop multiple notices stacking (#52746) Site Editor: Fix site link accessibility issues (#52744) ...
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 12, 2023
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 14, 2023
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 19, 2023
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 21, 2023
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Jan 12, 2024
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Jan 22, 2024
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Jan 22, 2024
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 12, 2024
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 15, 2024
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 16, 2024
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
Aschepikov
pushed a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 19, 2024
The issues were related to a change in Gutenberg where ActionCreators are not promisified. See WordPress/gutenberg#52530 [MAILPOET-5714]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
I've been working on integrating a
@wordpress/dataupdate into a different repo. There is an existing code snippet like this:However, this fails because
.thendoes not exist ondoSomething(e.g. the action creator doesn't return a promise)Why?
According to the documentation, this is a valid use case. However, our types prevent this because
doSomethingretains its original return type and isn't wrapped with a promise.This was also fixed in the DT types, but that fix never made it here (DefinitelyTyped/DefinitelyTyped#60693)
How?
I'm attempting to add a wrapper to our
ActionCreatorsOftype, which is used to generate return types fordispatch. This wrapper basically says that every action creator now returns aPromise<void>.Two things this doesn't handle:
Promise<void>ever returned, or is it alwaysvoid?Testing Instructions
unsure
Testing Instructions for Keyboard
n/a
Screenshots or screencast
n/a