Refactor: move `core/edit-post` INIT effect to use action-generators and controls by nerrad · Pull Request #14740 · WordPress/gutenberg

nerrad · 2019-04-01T12:53:17Z

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.

aduth

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

aduth · 2019-04-02T19:03:13Z

docs/designers-developers/developers/data/data-core-edit-post.md

-Returns an action object used signal a successful meta box update.
+Returns an action object used signal a successful meta box update.
+
+### init


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.

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

aduth · 2019-04-02T19:16:52Z

packages/edit-post/src/store/index.js


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


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

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 · 2019-04-02T20:53:27Z

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

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.

aduth · 2019-04-04T20:01:42Z

packages/edit-post/CHANGELOG.md

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


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

aduth · 2019-04-04T20:05:49Z

packages/edit-post/src/store/constants.js

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


I'd recommend some JSDoc for the constants.

nerrad · 2019-04-05T00:51:36Z

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

aduth · 2019-04-15T20:40:15Z

packages/edit-post/src/store/controls.js

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


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.

aduth · 2019-04-15T20:45:05Z

packages/edit-post/src/store/actions.js

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


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

oh ya, that would simplify things, good call.

Added the flag in 8287c2b and implemented in db6f2ca

aduth · 2019-04-18T15:09:47Z

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

This leaves room for future refactors that could eliminate these.

@aduth

props @aduth

nerrad · 2019-04-27T12:56:36Z

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 · 2019-04-27T13:04:14Z

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

gutenberg/packages/e2e-tests/specs/change-detection.test.js

Lines 215 to 229 in 87d7fe4

    
           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

LGTM 👍

aduth · 2019-05-02T16:37:11Z

packages/edit-post/src/store/constants.js

+ * 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';


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.

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

…nstableInitialize action. (fixes #15403) (#15444) Create EditorInitializer component and implement for various things to initialize as the editor is loaded. This replaces the `__unstableInitialize` refactor done in #14740.

nerrad requested review from aduth and jorgefilipecosta April 1, 2019 12:55

nerrad added the [Package] Edit Post /packages/edit-post label Apr 1, 2019

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

nerrad marked this pull request as ready for review April 1, 2019 12:58

nerrad requested review from ajitbohra, chrisvanpatten, gziolo, mkaz, noisysocks, notnownikki, oandregal, talldan and youknowriad as code owners April 1, 2019 12:58

aduth reviewed Apr 2, 2019

View reviewed changes

aduth reviewed Apr 4, 2019

View reviewed changes

nerrad force-pushed the refactor/edit-post-effects-to-controls-1 branch from 939fc9c to 0f663ee Compare April 5, 2019 00:50

noisysocks modified the milestones: 5.5 (Gutenberg), 5.6 (Gutenberg) Apr 17, 2019

aduth reviewed Apr 18, 2019

View reviewed changes

nerrad added 7 commits April 27, 2019 07:08

move constants into constants file

9729557

move init effect into action-generators and controls

5046796

add tests for new functionality

1f97ee1

update docs build

1cf604c

update changelog

f960196

clarify controls/actions that are experimental/unstable

6f937d9

This leaves room for future refactors that could eliminate these.

fix reference for version in changelog

4bd8154

nerrad added 3 commits April 27, 2019 07:08

move dispatch of initialize action to initializeEditor execution

f1d2a60

add js docs for constants

9c3fb4e

doc change

84ed847

nerrad force-pushed the refactor/edit-post-effects-to-controls-1 branch from 0f663ee to 84ed847 Compare April 27, 2019 11:08

nerrad added 4 commits April 27, 2019 08:02

Add flag for triggering initial invoking of listener

8287c2b

props @aduth

eliminate ADJUST_SIDEBAR control and move logic into action

db6f2ca

update tests

7b0ed6f

update docs

c1a5fe5

aduth approved these changes May 2, 2019

View reviewed changes

nerrad merged commit 920b8f0 into master May 2, 2019

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

aduth mentioned this pull request May 2, 2019

Edit Post: "View Post" link doesn't change when publishing draft #15402

Closed

This was referenced May 2, 2019

Refactor core/edit-post initialize action to a data component #15403

Closed

Refactor: Add data component for editor initialization to replace __unstableInitialize action. (fixes #15403) #15444

Merged

youknowriad modified the milestones: 5.6 (Gutenberg), 5.7 (Gutenberg) May 10, 2019

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

Conversation

nerrad commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Types of changes

Checklist:

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nerrad commented Apr 2, 2019

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nerrad commented Apr 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Apr 18, 2019

Uh oh!

nerrad commented Apr 27, 2019

Uh oh!

nerrad commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nerrad commented Apr 1, 2019 •

edited

Loading

nerrad commented Apr 27, 2019 •

edited

Loading