Add thunk actions to replace rungen and controls #27276
Conversation
|
Size Change: -156 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
try/async-await-actions
branch
from
59e269c to
b577371
Compare
|
It seems like a number of components relies on some actions (like |
|
Tests still fail, it might be more complex then. I will proceed here by changing one action and resolver at a time and observing the effects. |
jsnajdr
force-pushed
the
try/async-await-actions
branch
from
b577371 to
8a89237
Compare
jsnajdr
force-pushed
the
try/async-await-actions
branch
from
8a89237 to
d3eb6d2
Compare
|
I updated this PR to use a thunk-like API that's very similar to its counterpart in Redux. An action creator returns a function that is called by the store when the action is being dispatched, and receives const myAction = ( param ) => async ( { select, resolveSelect, dispatch, registry } ) => {
// call this store's selector, synchronously
const widgets = select.getWidgets();
// call a custom selector on this store, no need to make it part of public API
const posts = select( state => state.posts );
// dispatch an action
dispatch.createWidget( toWidget( param ) );
// some actions return a promise, let's await it
await dispatch.saveEditedWidgets();
// wait for selector resolution
const posts = await resolveSelect.getPosts();
// work with posts that are guaranteed to be fetched by now
workWith( posts );
}
// dispatch the action like any other
dispatch( 'mystore' ).myAction( theParam );This approach doesn't require any I adapted @adamziel's complete refactor of the |
I have good news! #28210 |
|
Love this @jsnajdr! This makes it easier to compose asynchronous and synchronous bits of code. Right now there's no real nice way to call multiple asynchronous actions at once. You have to use the global await Promise.all( [
dispatch.saveEntityRecord( ... ),
dispatch.saveEntityRecord( ... ),
] );I think using native How does this affect unit tests? The one advantage of using controls is that they're easy to test because it's just plain JavaScript objects that are |
|
The |
The import { create, asyncControls } from 'rungen';
const rungenRuntime = create( [ ...userControls, ...asyncControls ] );Then you could run a batch like this: import { fork, join } from 'rungen';
function* runBatch() {
const batch = yield createBatch();
const duneTask = yield fork( batch.add, { path: '/v1/books', title: ... } );
const lotrTask = yield fork( batch.add, { path: '/v1/books', title: ... } );
yield batch.run();
const duneResult = yield join( duneTask );
const lotrResult = yield join( lotrTask );
}
One downside is that The const value = yield Promise.resolve( 1 );
expect( value ).toBe( 1 );just with the built-in controls, without registering new ones. |
At this moment I'm exposing const coreDispatch = useDispatch( 'core/data' );
coreDispatch( { type: 'FOO' } );
const coreFoo = useSelect( select => select( 'core/data' )( state => state.foo ) );I'm not sure if that's a good idea, especially with I'll change the implementation to expose the |
That's true, if we want to test the thunks the same way we're testing the generators now, we'll need to mock most functions that are called by them. And then verify that these mocks got called with the expected arguments and in the expected order. I personally don't like this style of unit testing very much. It's easy to do, but it's more like cross-checking the internal implementation rather than testing its behavior. |
jsnajdr
force-pushed
the
try/async-await-actions
branch
from
5d48528 to
105b4af
Compare
|
I divided the patch into two parts:
The first part, adding the thunk middleware, is close to being mergeable. We only need to figure out how to flag the feature as experimental and make it opt-in for stores. Maybe with a flag like this? store = createReduxStore( 'core', {
reducer,
controls,
...,
__experimentalUseThunks: true
} );Without the The second part, migrating |
jsnajdr
force-pushed
the
try/async-await-actions
branch
from
105b4af to
d0ae93a
Compare
|
This is such a great progress @jsnajdr, thank you for pushing it forward!
The |
This sounds like a good use case for react-testing-library. We could have a minimal consumer for each action and selector, e.g. a component that's just a button saving an entity, a span displaying information fetched via |
My memory on that is hazy, but I think there was a corner case when a promise resolved with something resembling an action and rungen tried to dispatch it. I'm not sure though, it would be good to double check. |
Oh yes, that's indeed what will happen. The |
jsnajdr
force-pushed
the
try/async-await-actions
branch
from
d0ae93a to
ee3e0e0
Compare
jsnajdr
changed the title
|
This PR is now ready for review and merging. It adds a new experimental flag to data store config: store = createReduxStore( 'core', {
reducer,
controls,
...,
__experimentalUseThunks: true
} );When enabled, actions can be implemented as thunk functions. The |
|
I can't approve my own PR so cc @youknowriad @noisysocks :-) |
| @@ -102,47 +101,6 @@ export function createRegistry( storeConfigs = {}, parent = null ) { | |||
| return result; | |||
| } | |||
|
|
|||
| const getResolveSelectors = memize( | |||
There was a problem hiding this comment.
Does removing this usage of memize impact performance?
There was a problem hiding this comment.
I ran npm run test-performance with and without this branch and didn't see any regression.
There was a problem hiding this comment.
Good point. Removing memize impacts performance, but I don't think it's measurable in practice.
Without memize, the selectors are mapped (i.e., the object with resolution functions is created) from scratch every time registry.resolveSelect() is called. This is not needed, the mapping can be done only once.
I'm fixing this in #28544 by doing the mapping only once at store initialization.
Description
As the future of
@wordpress/stanis uncertain (#27109), I thought I'd experiment with removingrungendependency. This PR refactors a bunch of generator-based actions leveraging controls into regular asynchronous functions without dependency on controls. This is not time-sensitive and could be made obsolete by stan, but I just wanted to set things in motion here.A bunch of unit tests are going to fail simply because I did not adjust them. For now I'm more interested in e2e tests to see if such implementation breaks anything for the user.
Refactored use-cases:
Using selectors in actions
Before:
yield select( 'core', 'getEntityRecord', 'root', 'post', id );After:
select( 'core' ).getEntityRecord( 'root', 'post', id );Dispatching store-bound actions
Before:
yield dispatch( 'core', 'receiveEntityRecord', 'root', 'post', data );After:
await dispatch( 'core' ).receiveEntityRecord( 'root', 'post', data );Dispatching inline actions
Before:
yield { type: 'ACTION_NAME' }After:
await yieldAction( { type: 'ACTION_NAME' } )I'm not overly enthusiastic about this API, but I don't have any better ideas, other than maybe exposing every tiny action as a public API.
Deferring to generatorfunction (for compat)
Before:
yield* generatorFunction()After:
await yieldAction( generatorFunction() )A better way:
await dispatch( 'core' ).generatorAction();Using controls
Just do the task inline, e.g.:
Before:
After:
Notes
coreactions and resolvers were refactored in this PR - this shows how generators and async are interoperable, which is important in between landing PRs.reduxRoutimemiddleware and all the code related to rungen, controls, generators, etc.dispatch,select, andyieldActionviaregistryArgsthat's added via store creator. This should be okay for the first version, but ideally we wouldn't expose these functions globally.const data = yield anotherAction();could be entirely synchronous whileconst data await = await dispatch( 'store' ).anotherAction();will always be asynchronous. I am not sure if that's a problem though. First, we have locks API for when that matters. Second, yield-based dispatch could suddenly go asynchronous if anything in the call tree uses an async control. In other words, refactoring generator-based action A could have a ripple effect in seemingly unrelated action D that calls A via C and B.Related #26849 #27109
cc @jsnajdr @kevin940726 @noisysocks @draganescu @youknowriad @ellatrix