Refactor saveEntityRecord from redux-rungen to async thunks#33201
Refactor saveEntityRecord from redux-rungen to async thunks#33201
Conversation
|
Size Change: +105 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
thunkify/saveEntityRecord
branch
from
66eec97 to
abc2cac
Compare
adamziel
changed the title
|
The last missing part are the unit tests for |
adamziel
requested review from
ellatrix,
getdave,
gziolo,
noisysocks,
talldan and
youknowriad
|
Update the tests and 👍 lgtm. Doing this bit by bit makes sense. |
|
@adamziel I have a more ambitious version of this refactoring in #28389. It migrates the entire I just successfully rebased and updated the old version from January, and currently it compiles and I'm trying to make it work. I'm not sure yet which will be a better approach -- migrate one store at a time, or migrate individual generator functions? If we migrate one function at a time, I'm a bit worried whether the generators and thunks, the |
|
I just updated the tests, all the checks are green now! @jsnajdr as much as I'd love to do it in one swoop, I'm afraid that such a big PR targeting a place like that could get stuck in a long discussion/rebase loop. Every time e.g.
That was my concern too, but it looks like they play together pretty nicely – |
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
The great news is that this refactor is almost identical to my "in one swoop" version in #28389 🎉 That shows that we are on the right path and that incremental landing of this work is possible.
There's a bug in ifNotResolved that should be fixed.
Most other comments are about a fact that the thunk refactor reveals very well: some actions are synchronous, some are asynchronous and we need to await dispatches only of the asynchronous ones.
For saveEntityRecord, the two only async places are:
- waiting for the store lock
- issuing the
fetchrequest
Everything else is just updating the state in memory (all the receive... actions), and releasing a lock is also sync.
With thunks, dispatch( actionThunk ) is little more than a glorified function call that injects some dependencies into that function. And it's very clear, much clearer than with generators and yields, that if the thunk function is sync, the dispatch is also sync. And dispatching an action object ({ type: ..., ... }), once there are no controls, is also always sync.
| select.getEditedEntityRecord( kind, name, recordId ) | ||
| ); | ||
| yield editEntityRecord( | ||
| await dispatch.editEntityRecord( |
There was a problem hiding this comment.
My version doesn't do await here, because editEntityRecord is a synchronous action. It merely writes things to memory (updating state) and never does any request or anything else async.
| } | ||
|
|
||
| yield { | ||
| await dispatch( { |
There was a problem hiding this comment.
This action is also purely synchronous.
| await dispatch.receiveAutosaves( | ||
| persistedRecord.id, | ||
| updatedRecord | ||
| ); |
There was a problem hiding this comment.
Both receiveEntityRecords and receiveAutosaves are synchronous: they merely write the new/updated records to state.
| updatedRecord = await triggerFetch( options ); | ||
| } | ||
| yield receiveEntityRecords( | ||
| await dispatch.receiveEntityRecords( |
| } | ||
| yield { | ||
| dispatch( { | ||
| type: 'SAVE_ENTITY_RECORD_FINISH', |
| return updatedRecord; | ||
| } finally { | ||
| yield* __unstableReleaseStoreLock( lock ); | ||
| await dispatch( __unstableReleaseStoreLock( lock ) ); |
@adamziel I just found one limitation where In a generator, you can Rungen will also dispatch a generator/iterator: function* action() {
yield a;
yield b;
}and yield action();will execute that generator. But yielding a thunk doesn't work, because |
Done done, good spot!
I see the merit and value in that. At the same time, I see two problems:
On the flipside, the only cost of defaulting to Either way – I propose we get this PR in with all the |
@jsnajdr You're right! I think it would be possible to provide a handler as a rungen "control" (like the one it has for promises], but maybe we don't have to. It seems like:
Let's just keep working through the generators that are not yielded by any other generators (so leafs in the directed dependency graph) and hope we have no cyclic dependencies. Even if we do have cyclic deps, we could have one PR for each set of them. This way with every PR our dependency graph shrinks, and eventually we'll cover all the nodes. If we run into a case we can't handle that way, let's regroup. |
Yes, it would be possible to expand the But that could break backward compatibility. Until now, const x = yield () => 'x';assigned the function to As we're migrating away from |
adamziel
force-pushed
the
thunkify/saveEntityRecord
branch
from
ac3d052 to
4049925
Compare
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
Looks good to me now 👍 If all tests pass, this is ready to merge.
|
The only broken test is also broken in trunk – I'll wait for the performance checks and merge this one. Edit: I won't because e2e tests are now required :D Either someone can help me here (@gziolo ?), or this will have to wait until trunk is fixed. |
All the remaining ones are about
👍 |
…SpecifiedEntityEdits
…d add a test case for that
…romise, not a control descriptor
adamziel
force-pushed
the
thunkify/saveEntityRecord
branch
from
cb136b4 to
0989625
Compare
5 participants
Builds on top of the thunks support added in #27276 and refactors just the parts of core-data required to make the
saveEntityRecordwork (this PR is a minimal viable subset of #28389).Test plan:
So you ask how to test this PR? The impact surface is pretty big. For sure confirm all the tests are green – that is a good indicator of the overall health. You may want to play with the Gutenberg too:
blog titleblock, edit it, save the post and related entities