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: Add data component for editor initialization to replace __unstableInitialize action. (fixes #15403) #15444

Merged
merged 17 commits into from
Jun 25, 2019

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented May 5, 2019

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.

@nerrad nerrad added [Type] Enhancement A suggestion for improvement. [Package] Edit Post /packages/edit-post labels May 5, 2019
@nerrad nerrad self-assigned this May 5, 2019
@nerrad 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
blockSelectionListener,
adjustSidebarListener,
viewPostLinkUpdateListener,
} from './listeners';
Copy link
Member

@aduth aduth May 6, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :) )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nerrad nerrad Jun 11, 2019

Choose a reason for hiding this comment

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

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

@nerrad
Copy link
Contributor Author

nerrad commented May 22, 2019

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 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 nerrad marked this pull request as ready for review June 11, 2019 23:58
@nerrad nerrad added the [Type] Code Quality Issues or PRs that relate to code quality label Jun 11, 2019
const nodeToUpdate = useMemo(
() => document.querySelector( VIEW_AS_PREVIEW_LINK_SELECTOR ) ||
document.querySelector( VIEW_AS_LINK_SELECTOR )
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

nerrad commented Jun 12, 2019

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
Copy link
Contributor Author

nerrad commented Jun 12, 2019

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.

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.

This looks great. Nice work 👍

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


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

const previousOpenedSidebar = useRef( '' );
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nerrad nerrad Jun 26, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nerrad nerrad Jun 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nerrad nerrad Jun 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Edit Post /packages/edit-post [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants