-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update custom store readme to use thunks instead of controls #67006
Update custom store readme to use thunks instead of controls #67006
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
packages/data/README.md
Outdated
A **control** defines the execution flow behavior associated with a specific action type. This can be particularly useful in implementing asynchronous data flows for your store. By defining your action creator or resolvers as a generator which yields specific controlled action types, the execution will proceed as defined by the control handler. | ||
|
||
The `controls` option should be passed as an object where each key is the name of the action type to act upon, the value a function which receives the original action object. It should returns either a promise which is to resolve when evaluation of the action should continue, or a value. The value or resolved promise value is assigned on the return value of the yield assignment. If the control handler returns undefined, the execution is not continued. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what to do with the controls
section. Should it be refactored? Removed? I am not familiar with how controls are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hide it in a "details" kind of? and mark it as "deprecated"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried that in this commit: 6f24f40?short_path=f874ff4#diff-f874ff4a9c60876a3d01ac27950807424eead31c2cd914daa34136108bd03e0f. Let me know if that's not what you had in mind. I am not sure if it needs to be changed somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's basically what I had in mind. I wonder if will translate properly to the block editor handbook when published.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes perfect sense to mark the controls as deprecated 👍🏻
@@ -262,7 +253,7 @@ The data module shares many of the same [core principles](https://redux.js.org/i | |||
|
|||
The [higher-order components](#higher-order-components) were created to complement this distinction. The intention with splitting `withSelect` and `withDispatch` — where in React Redux they are combined under `connect` as `mapStateToProps` and `mapDispatchToProps` arguments — is to more accurately reflect that dispatch is not dependent upon a subscription to state changes, and to allow for state-derived values to be used in `withDispatch` (via [higher-order component composition](https://github.com/WordPress/gutenberg/tree/HEAD/packages/compose/README.md)). | |||
|
|||
The data module also has built-in solutions for handling asynchronous side-effects, through [resolvers](#resolvers) and [controls](#controls). These differ slightly from [standard redux async solutions](https://redux.js.org/advanced/async-actions) like [`redux-thunk`](https://github.com/gaearon/redux-thunk) or [`redux-saga`](https://redux-saga.js.org/). | |||
The data module also has built-in solutions for handling asynchronous side-effects, through [resolvers](#resolvers) and [thunks](https://github.com/WordPress/gutenberg/blob/trunk/docs/how-to-guides/thunks.md#thunks-can-be-async). These differ slightly from [standard redux async solutions](https://redux.js.org/advanced/async-actions) like [`redux-thunk`](https://github.com/gaearon/redux-thunk) or [`redux-saga`](https://redux-saga.js.org/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update this sentence or does it still apply?
These differ slightly from standard redux async solutions like
redux-thunk
orredux-saga
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsnajdr might know the answer. I don't think it's a blocker for merging, though.
Flaky tests detected in 6f24f40. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11844302508
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test the example but it looks good at first sight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test the example but it looks good at first sight.
Yes, it's difficult to test as the endpoint doesn't exist. There are some unit tests with examples like the following that I used for comparison:
gutenberg/packages/data/src/components/use-select/test/suspense.js
Lines 165 to 192 in a750125
const store = createReduxStore( 'test', { | |
reducer: ( state = {}, action ) => { | |
switch ( action.type ) { | |
case 'RECEIVE': | |
return { ...state, [ action.endpoint ]: action.data }; | |
default: | |
return state; | |
} | |
}, | |
selectors: { | |
getData: ( state, endpoint ) => state[ endpoint ], | |
}, | |
resolvers: { | |
getData: | |
( endpoint ) => | |
async ( { dispatch } ) => { | |
const delay = endpoint === 'slow' ? 30 : 10; | |
await new Promise( ( r ) => | |
setTimeout( () => r(), delay ) | |
); | |
dispatch( { | |
type: 'RECEIVE', | |
endpoint, | |
data: endpoint, | |
} ); | |
}, | |
}, | |
} ); |
Overall, it's a good improvement that promotes thunks.
I think that's always been a barrier for consumers who wanted to get started with data stores. The primary example should be copy-pastable into plugin files to get you started. We could follow up on this separately. Maybe @ryanwelcher has some ideas :) |
What?
Update the custom store explanation to use thunks instead of controls.
Why?
If I am not mistaken, now that thunks are supported, it should be the recommended way of handling these use cases as explained here.
How?
I just updated the code examples in the readme.