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

nerrad · 2019-05-05T22:58:40Z

Description

This addresses #15403

In #14740 the INIT effect was refactored to an action generator using controls. However as per this comment it was noted that a flaw existing with the original effect and carried over to the new action-generator is that the subscription can/will never be unsubscribed.

In this pull is the introduction of a <EditorInitialization /> renderless component (render returning null) with the task of carrying out any initialization tasks for the Editor component. It replaces the __unstableInitialization action creator and the dispatches happening in initializeEditor.

Some advantages here:

Implementation of useSelect and useDispatch in custom hooks to handle the logic previously contained in custom subscribers.
This utilizes the registry exposed through the RegistryProvider (via the useSelect and useDispatch hooks).
The new component is within the ErrorBoundary, so any problems get caught by that boundary.

How has this been tested?

I added tests for the new hooks (which is basically replicating the tests I added for the __unstableInitialize action). These should pass.
Existing unit tests should pass.
Changes are covered by e2e tests so those should pass as well.

Todo

Extract @wordpress/data changes into its own pull (@wordpress/data: Export entire registry context #15445) (this was superseded by the useSelect and useDispatch work)
When ^^ is merged to master, remove cherry-picked code in here and implement directly.
Update CHANGELOG.md with link to this pull.

Types of changes

This is a non-breaking change. Although there is the removal of an action, at the time of this pull the action was not only labelled __unstable but the action had not even been published yet.

There should be minimal impact to any other code in the editor because fundamentally it's just shifting how things are initialized.

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.

packages/edit-post/src/components/editor-initialization/index.js

aduth · 2019-05-06T13:31:11Z

packages/edit-post/src/components/editor-initialization/index.js

+	blockSelectionListener,
+	adjustSidebarListener,
+	viewPostLinkUpdateListener,
+} from './listeners';


Rather than listener functions called by a single subscription, could we have each of these behaviors implemented in their own components and re-use existing tools like withSelect to feed data (to which updates occur using either componentDidUpdate or a hooks equivalent)?

Prior art:

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/multi-select-scroll-into-view/index.js

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/preserve-scroll-in-reorder/index.js

https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/components/browser-url/index.js

https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/components/fullscreen-mode/index.js

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/autosave-monitor/index.js

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/unsaved-changes-warning/index.js

We could, I'm keen on understanding what advantages this provides over the approach in this pull (there's probably something I'm missing because its not immediately obvious to me :) )?

So one benefit that is apparent is it helps keep things more singular in purpose?

So one benefit that is apparent is it helps keep things more singular in purpose?

Yes, I think it helps keep the behaviors single-focused. It also allows us to reuse existing, well-understood patterns (withSelect) than to create custom subscriber implementations.

This has been refactored to now use custom hooks (implementing useSelect and useDispatch).

nerrad · 2019-05-22T00:54:32Z

Just noting that work on this pull is on hold in anticipation of the work in #15737 and #15473 aiding with a more robust solution.

nerrad · 2019-06-12T00:10:57Z

packages/edit-post/src/components/editor-initialization/listener-hooks.js

+	const nodeToUpdate = useMemo(
+		() => document.querySelector( VIEW_AS_PREVIEW_LINK_SELECTOR ) ||
+			document.querySelector( VIEW_AS_LINK_SELECTOR )
+	);


I'm rethinking the use of useMemo here. From the react docs:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

So instead it may be preferable to make nodeToUpdate a ref and assign it on a single run useEffect. I'll implement that.

nerrad · 2019-06-12T01:22:15Z

The e2e test fails (nux tips) are legit, I'm reproducing in testing as well. I'll investigate how the changes here might have impacted that.

nerrad · 2019-06-12T02:31:32Z

Alright this is ready for review. I ran performance tests and there's not really much difference between this pull and master (as expected). The only failing job with travis is a phpcs linting fail. Since nothing in this pull touched php it likely came from upstream.

When this passes code review I think this is ready to go.

aduth

This looks great. Nice work 👍

There's a merge conflict which needs to be resolved.

aduth · 2019-06-25T20:32:56Z

packages/edit-post/src/components/editor-initialization/listener-hooks.js

+
+	const { openGeneralSidebar, closeGeneralSidebar } = useDispatch( STORE_KEY );
+
+	const previousOpenedSidebar = useRef( '' );


I might think null serves better as representing an unset value (this is also what was used previously). Unless for some reason null can't be used as the argument of useRef?

I used empty string because of a habit I have of using the same type for empty values (previousOpenedSidebar.current would have a string value). I think it's a good signal of what type to expect?

I used empty string because of a habit I have of using the same type for empty values (previousOpenedSidebar.current would have a string value). I think it's a good signal of what type to expect?

Hm, to me, we're representing an explicitly empty value, which is the purpose that a null type serves (reference), and isn't really a semantic guarantee of an empty string.

The other option we may have here is to not set any value (just initialize with useRef())?

Although that may mean previousOpenedSidebar.current will be undefined (I'd have to check).

Edit: just confirmed useRef() results in the expected { current: undefined } value.

Yeah, I'd still think null would be preferred over undefined, as the distinction between the two is in intentionality / explicitness of emptiness (vs. merely "unset").

However, in your reference is stated:

represents the intentional absence of any object value

And the accompanying example illustrates where null is used to reference an empty value for something that would return an object otherwise. That's typically how I use null as the initial value for a variable, when the type returned would be an object as opposed to a primitive. Of course this isn't a hard and fast "rule" because context is important (i.e. it may not always be good to initialize value that represents a number primitive with 0) but in general I find value when reading code with initialized values that communicate expected type.

when the type returned would be an object as opposed to a primitive

I'm not sure I follow. You're suggesting to distinguish it based on whether it's typeof 'object' vs. some other primitive type?

Right. In this case previousOpenedSidebar.current will always return a string. So it's empty representation would be an empty string (keeping type equality). If previousOpenedSidebar.current is expected to return an object, then it's empty representation would be null (which seems to be the use-case for how MDN describes using null).

On line 75 I also followed the same line of thought by resetting previousOpenedSidebar.current to an empty string:

gutenberg/packages/edit-post/src/components/editor-initialization/listener-hooks.js

Line 75 in 521664c

previousOpenedSidebar.current = '';

I think null would be also be an appropriate initial value to use in the case where a value may represent any number of data types.

packages/edit-post/src/components/editor-initialization/index.js

important on the offchance the post is not retrieved (prevents undefined errors in the case `post.id` doesn’t exist).

… initiatlization component

nerrad added [Type] Enhancement A suggestion for improvement. [Package] Edit Post /packages/edit-post labels May 5, 2019

nerrad self-assigned this May 5, 2019

nerrad commented May 5, 2019

View reviewed changes

packages/edit-post/src/components/editor-initialization/index.js Outdated Show resolved Hide resolved

nerrad mentioned this pull request May 5, 2019

@wordpress/data: Export entire registry context #15445

Closed

6 tasks

nerrad changed the title ~~Refactor: Add data component for editor initialization to replace __unstableInitialize action.~~ Refactor: Add data component for editor initialization to replace __unstableInitialize action. (fixes #15403) May 6, 2019

aduth reviewed May 6, 2019

View reviewed changes

nerrad force-pushed the refactor/data-component-for-editor-initialization branch 2 times, most recently from 789f4b7 to cf5e582 Compare June 11, 2019 23:53

nerrad marked this pull request as ready for review June 11, 2019 23:58

nerrad requested review from talldan and youknowriad as code owners June 11, 2019 23:58

nerrad added the [Type] Code Quality Issues or PRs that relate to code quality label Jun 11, 2019

nerrad commented Jun 12, 2019

View reviewed changes

aduth approved these changes Jun 25, 2019

View reviewed changes

nerrad added 12 commits June 25, 2019 19:30

automatic update to readme for @wordpress/data

17b9283

no idea why this is getting changes

4d7a9c2

add new EditorInitialization data component (and tests)

ee2f8aa

implement new component in the Editor setup

8f81af4

remove obsolete code replaced by new component

03041b9

dunno why this keeps reporting changes

03acbf3

use the postId prop

84ab1f4

important on the offchance the post is not retrieved (prevents undefined errors in the case `post.id` doesn’t exist).

update changelog with link to pull.

973f7f8

add constant for preview link selector

4a602b8

add hooks for editor intitialization actions and implement in special…

301e675

… initiatlization component

update changelog

4e11e87

expand jsdocs to hopefully trigger travis build.

508e222

nerrad added 5 commits June 25, 2019 19:32

remove obsolete tests

d5e9efd

switch away from useMemo

68f680f

doh! Add back in initial trigger of nux guides!

09db379

fix spacing issues after merge from master

fd56104

fix order of imports

521664c

nerrad force-pushed the refactor/data-component-for-editor-initialization branch from 6c4872b to 521664c Compare June 25, 2019 23:38

nerrad merged commit 1414cf0 into master Jun 25, 2019

nerrad deleted the refactor/data-component-for-editor-initialization branch June 25, 2019 23:51

nerrad added this to the Gutenberg 6.1 milestone Jun 26, 2019

This was referenced Oct 2, 2019

Pinned plugin sidebars are visible in iPad portrait, but not clickable #17623

Closed

Plugins: Ensure sidebar plugins do not get auto-closed when opened on… #17712

Merged

obenland mentioned this pull request Sep 15, 2020

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

Closed

This was referenced Jul 16, 2024

[Snyk] Upgrade @wordpress/dependency-extraction-webpack-plugin from 3.7.0 to 6.1.0 WontonSam/woocommerce-admin#129

Open

[Snyk] Upgrade @wordpress/element from 4.4.1 to 6.1.0 WontonSam/chicommerce#284

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nerrad commented May 5, 2019 •

edited

Loading

aduth May 6, 2019 •

edited

Loading

nerrad May 6, 2019

nerrad May 6, 2019

aduth May 6, 2019

nerrad Jun 11, 2019 •

edited

Loading

nerrad commented May 22, 2019

nerrad Jun 12, 2019

nerrad commented Jun 12, 2019

nerrad commented Jun 12, 2019

aduth left a comment

aduth Jun 25, 2019

nerrad Jun 25, 2019

aduth Jun 26, 2019

nerrad Jun 26, 2019 •

edited

Loading

nerrad Jun 26, 2019 •

edited

Loading

aduth Jun 26, 2019

nerrad Jun 26, 2019 •

edited

Loading

aduth Jun 27, 2019

nerrad Jun 27, 2019


		const { openGeneralSidebar, closeGeneralSidebar } = useDispatch( STORE_KEY );

		const previousOpenedSidebar = useRef( '' );

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

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

Conversation

nerrad commented May 5, 2019 • edited Loading

Description

How has this been tested?

Todo

Types of changes

Checklist:

aduth May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

nerrad commented May 22, 2019

Choose a reason for hiding this comment

nerrad commented Jun 12, 2019

nerrad commented Jun 12, 2019

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

nerrad Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad commented May 5, 2019 •

edited

Loading

aduth May 6, 2019 •

edited

Loading

nerrad Jun 11, 2019 •

edited

Loading

nerrad Jun 26, 2019 •

edited

Loading

nerrad Jun 26, 2019 •

edited

Loading

nerrad Jun 26, 2019 •

edited

Loading