Skip to content
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

Refactor setupEditor effects to actions #14513

Merged
merged 7 commits into from
Mar 22, 2019

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented Mar 19, 2019

Description

This moves the setupEditor effects in the core/editor store into action-generators. Pretty straightforward change.

Also removes redux-multi dependency because setupEditor was the only effect using it.

How has this been tested?

  • unit test modifications/ensure they pass.
  • Manual testing of various editor states to ensure behaviour hasn't changed
  • ensure no e2e breakage

Screenshots

Types of changes

Non-breaking enhancement.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@nerrad nerrad self-assigned this Mar 19, 2019
@nerrad nerrad added the [Package] Editor /packages/editor label Mar 19, 2019
@nerrad nerrad added this to the 5.4 (Gutenberg) milestone Mar 19, 2019
@nerrad nerrad added the [Type] Enhancement A suggestion for improvement. label Mar 19, 2019
@nerrad nerrad marked this pull request as ready for review March 19, 2019 15:15
@nerrad nerrad requested a review from aduth March 19, 2019 15:15
@aduth
Copy link
Member

aduth commented Mar 20, 2019

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 refx. In other words, we either need some other way to include those middlewares in the copied registry store, or to eliminate the middlewares altogether. My preference (supported by this pull request) would be the latter.

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 [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you mean that we can remove the redux-multi dependency because of this (which makes sense).

Yes, this.

Copy link
Contributor Author

@nerrad nerrad Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nerrad
Copy link
Contributor Author

nerrad commented Mar 20, 2019

or to eliminate the middlewares altogether. My preference (supported by this pull request) would be the latter.

Ya that would be mine as well. Keeping the dependency tree as small as possible is a good goal.

@nerrad nerrad force-pushed the refactor/move-setup-editor-effect-to-controls branch from 1a5a0bb to 1a827a4 Compare March 21, 2019 00:36
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely 👍

(Though I'd still recommend to optionally consider my review notes)

* the specified post object and editor settings.
*
* @param {Object} post Post object.
* @param {Object} edits Initial edited attributes object.
* @param {Array?} template Block Template.
*
* @return {Object} Action object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

http://usejsdoc.org/tags-yields.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added @yields as a feature request for the docgen default formatter: #14583

packages/editor/src/store/actions.js Outdated Show resolved Hide resolved
@nerrad nerrad force-pushed the refactor/move-setup-editor-effect-to-controls branch from 1a827a4 to 46a53e0 Compare March 22, 2019 14:58
@nerrad nerrad merged commit 3f28c95 into master Mar 22, 2019
@nerrad nerrad deleted the refactor/move-setup-editor-effect-to-controls branch March 22, 2019 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Editor /packages/editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants