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: move `core/edit-post` INIT effect to use action-generators and controls #14740

Merged
merged 14 commits into from May 2, 2019

Conversation

@nerrad
Copy link
Contributor

commented Apr 1, 2019

Description

As a part of the ongoing effort to refactor effects to controls, this is the first pull to tackle the core/edit-post store. Pulls will be granular to aid with easier, shorter reviews.

In this pull the INIT efffect has been refactored. Some noteworthy things in this pull:

  • introduction of a subscribe control that gives an idea for how it can be implemented.
  • introduction of a specific control for adjusting sidebars. I chose to do this because of it's complexity and to retain current behaviour.
  • The action-generator is now much more simplified and testable.
  • The previous INIT effect had no unit-tests (understandably). With these changes its now much easier to mock the registry and test the logic of initialization.

How has this been tested?

  • Ensure existing and new unit-tests pass.
  • Ensure existing e2e tests pass

This pull impacts the following things:

  • toggling the edit-post/block general sidebar when a block is selected/un-selected. Can be tested by selecting a block, the sidebar should switch from "Document" to "Block".
  • hiding/showing the sidebar depending on size of viewport. Can be tested by resizing the browser window and verifying that at the mobile breakpoint the sidebar's disappear/appear as you resize.
  • updates "View Post" link in the admin menu bar when permalink is updated for a post. Can be tested by editing a published post and editing the permalink. Verify the "View Post" link is updated with the new permalink.

Types of changes

This is a non-breaking change.

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 requested review from jorgefilipecosta and aduth Apr 1, 2019

@nerrad nerrad added this to the 5.5 (Gutenberg) milestone Apr 1, 2019

@nerrad nerrad marked this pull request as ready for review Apr 1, 2019

@aduth
Copy link
Member

left a comment

My initial reaction is: While the subscribe control is the most faithful adaptation of the current implementation, is it something where we might want to lean instead on other patterns around subscribed component behaviors, optionally as data-only components (example)?

One concern with the proposed subscriptions (and the same applies for the current implementation): How do these handlers become unsubscribed (e.g. in a potential future more single-page app long-lived sessions). I could grant that implementing this as a component is slightly more cumbersome for what amounts to the closest semblance of a true "side effect", but it at least has a built-in mechanism for subscription lifetime being tied to that of the app element root (if considering initialize as simply the public interface to mount an <EditPost /> component).

cc @youknowriad

Returns an action object used signal a successful meta box update.

### init

This comment has been minimized.

Copy link
@aduth

aduth Apr 2, 2019

Member

I might recommend initialize, both for (a) alignment to the wp.editor.initializeEditor function and (b) to avoid abbreviations which aren't always obvious to the reader.

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 2, 2019

Author Contributor

Yes, that makes sense. I only went with init because it matched the original side-effect name INIT.

persist: [ 'preferences' ],
} );

applyMiddlewares( store );
store.dispatch( { type: 'INIT' } );
store.dispatch( actions.init() );

This comment has been minimized.

Copy link
@aduth

aduth Apr 2, 2019

Member

Is there any challenge in moving this dispatch to be as part of the initialize function in edit-post/src/index.js?

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 2, 2019

Author Contributor

I don't know. Certainly could try it. I only did it here to preserve existing behaviour as much as possible. The refactors are already a bit of a chore without considerations for other improvements that could be made. So one the goals with these refactors are:

  • preserve existing behaviour as much as possible
  • only make changes if it's necessary to keep things working or if there are clear obvious non-complex benefits to doing so.

Certainly, it would make sense to do the dispatch on initialize though, so I could give it a go (especially since its exposed as a action whereas before INIT was only a side effect encapsulated in the store - hence why it was probably here).

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

My initial reaction is: While the subscribe control is the most faithful adaptation of the current implementation, is it something where we might want to lean instead on other patterns around subscribed component behaviors, optionally as data-only components (example)?
One concern with the proposed subscriptions (and the same applies for the current implementation): How do these handlers become unsubscribed (e.g. in a potential future more single-page app long-lived sessions). I could grant that implementing this as a component is slightly more cumbersome for what amounts to the closest semblance of a true "side effect", but it at least has a built-in mechanism for subscription lifetime being tied to that of the app element root (if considering initialize as simply the public interface to mount an component).

I thought something similar as I did this refactor. But as noted in another comment, I've imposed a limit on these refactors to preserve existing behaviour in the implemented control/action. Certainly there's room for further re-thinking on this and implementing something like a data component as you pointed out in another iteration after current effects have been eliminated.

Doing these refactors certainly does raise the profile of some of these lingering issues in the current stable of effects.

@aduth
Copy link
Member

left a comment

But as noted in another comment, I've imposed a limit on these refactors to preserve existing behaviour in the implemented control/action.

That's fair. I appreciate the pushback in limiting the scope to solving one problem (effects) and not all problems (initialization flow).

On that then, my only hesitation would be about impact to the public API. If we're not certain we want to keep around an initialization action, then we should mark it as either experimental or unstable. For something like the subscribe controls, while I'm not sure I'd want to see it develop into a common pattern (there was some recent mention of something of a data-controls package for common controls), I think it'd be fine enough as an internal implementation detail here in the meantime.

@@ -1,9 +1,13 @@
## V.V.V (Unreleased)
## 3.3.0 (Unreleased)

This comment has been minimized.

Copy link
@aduth

aduth Apr 4, 2019

Member

Per this week's Core JS chat, we should leave this as unset (or change to "Unreleased" or "Master").

@@ -0,0 +1,2 @@
export const STORE_KEY = 'core/edit-post';
export const VIEW_AS_LINK_SELECTOR = '#wp-admin-bar-view a';

This comment has been minimized.

Copy link
@aduth

aduth Apr 4, 2019

Member

I'd recommend some JSDoc for the constants.

@nerrad nerrad force-pushed the refactor/edit-post-effects-to-controls-1 branch from 939fc9c to 0f663ee Apr 5, 2019

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@aduth this was rebased and I addressed all your comments. I think this is pretty much ready for merge (assuming e2e tests pass).

* receives the registry.
* @return {Object} control descriptor.
*/
export function __unstableSubscribe( listenerCallback ) {

This comment has been minimized.

Copy link
@aduth

aduth Apr 18, 2019

Member

Since it's not exposed on a public API, I suppose it doesn't strictly need the prefix. But could be a good indicator for internal use as well.

}
) );
// hide/show the sidebar depending on size of viewport.
yield __experimentalAdjustSidebar();

This comment has been minimized.

Copy link
@aduth

aduth Apr 18, 2019

Member

Is the main issue with not being able to inline the implementation here like with other subscribers that it needs to have an initial call to adjuster? I'd wonder if there might be a simpler option to creating the separate action creator and controls, in enhancing onChangeListener to accept an optional argument to trigger an initial call to the callback.

diff --git a/packages/edit-post/src/store/utils.js b/packages/edit-post/src/store/utils.js
index 86b043327..3e0d287dc 100644
--- a/packages/edit-post/src/store/utils.js
+++ b/packages/edit-post/src/store/utils.js
@@ -2,12 +2,19 @@
  * Given a selector returns a functions that returns the listener only
  * if the returned value from the selector changes.
  *
- * @param  {function} selector Selector.
- * @param  {function} listener Listener.
- * @return {function}          Listener creator.
+ * @param {Function} selector Selector.
+ * @param {Function} listener Listener.
+ * @param {boolean}  initial  Whether to call listener immediately.
+ *
+ * @return {Function} Listener creator.
  */
-export const onChangeListener = ( selector, listener ) => {
+export const onChangeListener = ( selector, listener, initial ) => {
 	let previousValue = selector();
+
+	if ( initial ) {
+		listener( previousValue );
+	}
+
 	return () => {
 		const selectedValue = selector();
 		if ( selectedValue !== previousValue ) {

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 27, 2019

Author Contributor

oh ya, that would simplify things, good call.

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 27, 2019

Author Contributor

Added the flag in 8287c2b and implemented in db6f2ca

@aduth

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Sorry, I had the previous two comments as pending, and thought I'd submitted them earlier. I don't want this to be held up, but I do wonder if the small revision to onChangeListener could help provide an overall simpler approach (and fewer controls, in mind of some prior feedback about the desired nature of controls).

nerrad added some commits Apr 1, 2019

clarify controls/actions that are experimental/unstable
This leaves room for future refactors that could eliminate these.

nerrad added some commits Apr 5, 2019

@nerrad nerrad force-pushed the refactor/edit-post-effects-to-controls-1 branch from 0f663ee to 84ed847 Apr 27, 2019

nerrad added some commits Apr 27, 2019

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

There's a failing e2e test that doesn't appear to be related to any work in this branch, but restarting the test doesn't seem to fix it. It's not immediately clear to me what the test is supposed to cover (I don't know what a "dirty dialog" is) so I can't access whether it's an actual failing issue from this branch.

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Well looks like restarting it two times did the trick. The test that was failing was:

it( 'Should prompt if changes and save is in-flight', async () => {
await page.type( '.editor-post-title__input', 'Hello World' );
// Hold the posts request so we don't deal with race conditions of the
// save completing early. Other requests should be allowed to continue,
// for example the page reload test.
await interceptSave();
// Keyboard shortcut Ctrl+S save.
await pressKeyWithModifier( 'primary', 'S' );
await releaseSaveIntercept();
await assertIsDirty( true );
} );

@aduth

aduth approved these changes May 2, 2019

Copy link
Member

left a comment

LGTM 👍

* CSS selector string for the admin bar view post link anchor tag.
* @type {string}
*/
export const VIEW_AS_LINK_SELECTOR = '#wp-admin-bar-view a';

This comment has been minimized.

Copy link
@aduth

aduth May 2, 2019

Member

Aside: It wasn't introduced here, but "View" shows for draft posts with a different ID wp-admin-bar-preview (view vs. preview) and thus wouldn't be updated.

This comment has been minimized.

Copy link
@aduth

aduth May 2, 2019

Member

Aside: It wasn't introduced here, but "View" shows for draft posts with a different ID wp-admin-bar-preview (view vs. preview) and thus wouldn't be updated.

Follow-up issue: #15402

@nerrad nerrad merged commit 920b8f0 into master May 2, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the refactor/edit-post-effects-to-controls-1 branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.