Refactor setupEditor effects to actions#14513
Conversation
nerrad
requested review from
ajitbohra,
chrisvanpatten,
gziolo,
mkaz,
noisysocks,
notnownikki,
oandregal,
talldan and
youknowriad
as code owners
|
In some ways this can be considered a blocker for #14367, as otherwise the creation of a separate registry doesn't account for the custom middlewares we apply; one of which being I'm experimenting with your commits cherry-picked into a local variation of #14367, and will circle back here with those findings supporting a review. |
| blocks = synchronizeBlocksWithTemplate( blocks, template ); | ||
| } | ||
|
|
||
| return [ |
There was a problem hiding this comment.
I suspect that this was the only occurrence of an array-action for which we apply the redux-multi middleware (i.e. upon confirmation, it may be warranted to remove as part of these changes).
There was a problem hiding this comment.
Sorry I don't follow: The refactor changed this from an array action to dispatch actions. So the work in this pull already removes the reliance on the redux-multi middleware for setupEditor? Or do you mean that we can remove the redux-multi dependency because of this (which makes sense).
There was a problem hiding this comment.
Or do you mean that we can remove the redux-multi dependency because of this (which makes sense).
Yes, this.
There was a problem hiding this comment.
Doesn't look like anything else was using this in the editor package so went ahead and removed it. I think I did things correctly (just removed from package.json) but with this being a mono-repo I'm not sure :)
removed in 52af9b3
There was a problem hiding this comment.
I think I did things correctly (just removed from package.json) but with this being a mono-repo I'm not sure :)
The one-line removal from package-lock.json seems correct since it's still a dependency in block-editor (though perhaps it shouldn't be; a point outside the scope of this pull request).
In any case, Travis would have caught it anyways if it was an issue, and it appears to be just fine.
Ya that would be mine as well. Keeping the dependency tree as small as possible is a good goal. |
nerrad
force-pushed
the
refactor/move-setup-editor-effect-to-controls
branch
from
1a5a0bb to
1a827a4
Compare
| * @param {Object} edits Initial edited attributes object. | ||
| * @param {Array?} template Block Template. | ||
| * | ||
| * @return {Object} Action object. |
There was a problem hiding this comment.
Nit: I'd wondered if JSDoc had some option to document generator function yields. It appears @yields is a thing, a suitable replacement for @return.
There was a problem hiding this comment.
Ya I had thought about using @yields but the difficulty here is that most if not all of these actions do not have a single yield. So it's a bit tricky knowing what exactly to document.
I've sort of loosely followed the pattern of only include a @return tag on generators when there are return values in the generator, otherwise having nothing.
There was a problem hiding this comment.
Added @yields as a feature request for the docgen default formatter: #14583
nerrad
force-pushed
the
refactor/move-setup-editor-effect-to-controls
branch
from
1a827a4 to
46a53e0
Compare
3 participants
Description
This moves the
setupEditoreffects in thecore/editorstore into action-generators. Pretty straightforward change.Also removes
redux-multidependency becausesetupEditorwas the only effect using it.How has this been tested?
Screenshots
Types of changes
Non-breaking enhancement.
Checklist: