Try adding a pure Higher Order Component by youknowriad · Pull Request #6313 · WordPress/gutenberg

youknowriad · 2018-04-20T10:41:38Z

This PR adds a purify Higher Order Component used to wrap withSelect and withDispatch components (for now) to make them pure.

Pure components mean they don't rerender unless their props or state changed (shallow equality).

Testing instructions

Turn the lights on (the "highlight updates" in the React dev tools panel)
Compare the lights when typing in a block with master

Notes

Using "pure" pattern extensively means we're adding "checks" computation in favor of rendering computation. I think globally it's more performant because we do only shallow comparison but this is something to keep in mind.

atimmer · 2018-04-20T11:14:57Z

Would using PureComponent from React be an option?

youknowriad · 2018-04-20T11:24:56Z

@atimmer Not certain what's the difference between the two implementation, do you know more about it?

I kind of like the HoC approach, where you can use it without transforming your component into a class. Also, I don't think it's possible to use React.PureComponent the way I'm implementing purify: A higher component above any class extending wp.element.Component.

gziolo · 2018-04-20T13:01:54Z

From https://reactjs.org/docs/react-api.html#reactpurecomponent:

React.PureComponent’s shouldComponentUpdate() only shallowly compares the objects. If these contain complex data structures, it may produce false-negatives for deeper differences. Only extend PureComponent when you expect to have simple props and state, or use forceUpdate() when you know deep data structures have changed. Or, consider using immutable objects to facilitate fast comparisons of nested data.

Indeed it's interesting how you handle Component class. It doesn't add another wrapper component. Technically you could use PureComponent to wrap functional components, but it probably would end up the same.

Furthermore, React.PureComponent’s shouldComponentUpdate() skips prop updates for the whole component subtree. Make sure all the children components are also “pure”.

Not sure about this part. I would expect children components to re-render when they are wrapped with data HOCs anyway.

gziolo · 2018-04-20T13:03:00Z

@youknowriad can we add some tests once we agree that it is the way to go to make sure that purify never regresses?

mcsf · 2018-04-20T13:28:19Z

Minor: have you considered naming it pure? This follows convention for other HoCs, whose names are not actions, but rather predicates applied to the component:

this is a component with safe timeout
this is a component [made] pure

mcsf · 2018-04-20T13:32:31Z

I generally approve this.

I agree tests, esp. for regressions, are in order. Further: although making some critical components pure should help us a lot, there's the caveat that this HoC may be abused in the future if not applied judiciously (like other kinds of premature or uninformed optimization). Short of improving HoC documentation, I don't really know what the solution for that is. :)

aduth

I'd also prefer the name pure.

aduth · 2018-04-20T13:33:20Z

data/index.js

 * @return {Component} Enhanced component with merged state data props.
 */
 export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( WrappedComponent ) => {
-	return class ComponentWithSelect extends Component {


Can this be expressed as:

createHigherOrderComponent( compose( [ purify, ( WrappedComponent ) => class extends Component { } ] ), 'withSelect' );

aduth · 2018-04-20T13:34:16Z

element/index.js

+ *
+ * @return {WPComponent} Component class with generated display name assigned.
+ */
+export const purify = createHigherOrderComponent( ( Wrapped ) => {


Why did you choose to export this in element, and not components ?

FWIW, I had a branch with similar change, and encountered that data would then depend on components, which surfaced yet another issue of: Should withSelect and withDispatch be in data or not?

I asked myself several times about withFilters in components which depends on hooks. A very similar case.

Why did you choose to export this in element, and not components ?

I'm sure this is all about dependencies. There needs to be a solution :)

I actually didn't think that much about it. I put it under element to match React :) which also provides a pure component definition.

Well explained, I'm sold :)

Hmm, ignoring the fact that React does it, I don't see why, for example, we'd want to export a pure higher-order component in element, but ifCondition higher-order component in components. What judgement do we use to categorize them?

pure, ifCondition, withState, withInstanceId, but also createHigherOrderComponent and compose - we can put all of them together in their own module 💯

The alternative perspective is that components is a baseline, and where we're encountering circular dependencies is a sign where we have our dependencies inversed. Considering viewport and the issues highlighted in #5316, the current dependency hierarchy is:

Viewport (ifViewportMatches) > Components (ifCondition)

(This is problematic in #5316 in introducing the reverse dependency from Popover to Viewport)

Where maybe it ought to be:

Components (ifViewportMatches + ifCondition) > Viewport (withSelect( 'viewport' ) )

This highlights a third type of component: Data-bound components.

We could treat them all separately, e.g. higher-order-components, app-components, ui-components, though it's not clear to me whether this will ultimately solve all of our issues, and of course has added overhead in making determinations of which components / utilities belong where.

So did we just not decide anything here?

I assumed it's going to be renamed to pure.

It was ... I guess you are referring to the location of this HOC, we need to come up with something sooner than later 👍

aduth · 2018-04-20T13:36:24Z

element/index.js

+
+	return class extends Component {
+		shouldComponentUpdate( nextProps ) {
+			return ! isShallowEqual( nextProps, this.props );


Interesting conclusion that we don't need to compare state here, and I suppose it makes sense since it's wrapping the original (non-class) component.

Agreed on this one, there is no state in functional components 👍

gziolo · 2018-04-20T14:56:44Z

element/test/index.js

Typo in the comment (also in line 246).
How this wrapper.update() works, it's super confusing ...

Well I wanted to rerender the component without passing new props or something. But it's not that important since passing a similar prop is also tested

gziolo · 2018-04-25T15:07:02Z

I would love to see this PR merged later this week. I guess the only remaining issue is where this new functionality should be located as discussed here: #6313 (comment). Otherwise, it has big 👍 from me :)

aduth · 2018-04-25T15:10:45Z

I've published @wordpress/is-shallow-equal and will bring it in for use here unless you see reason not to.

mcsf

🎸

youknowriad · 2018-04-27T09:54:10Z

@aduth Are you planning to update this PR or use @wordpress/is-shallow-equal in a separate PR? To know whether I should merge or wait :)

aduth · 2018-04-27T15:48:17Z

@youknowriad In the process of integrating, I found a bug which led to WordPress/packages#116 . We could merge that and bring it in here, or do separately. I'd be fine to do separately as well.

youknowriad self-assigned this Apr 20, 2018

youknowriad requested review from aduth and mcsf April 20, 2018 10:41

youknowriad added the [Type] Performance Related to performance efforts label Apr 20, 2018

Try adding a purify Higher Order Component

1f428d3

youknowriad force-pushed the add/purifyhoc branch from 33c838e to 1f428d3 Compare April 20, 2018 11:02

gziolo added this to the 2.8 milestone Apr 20, 2018

aduth reviewed Apr 20, 2018

View reviewed changes

Rename purify => pure

f5acd8b

gziolo reviewed Apr 20, 2018

View reviewed changes

Unit test the pure HoC

67e17f6

youknowriad force-pushed the add/purifyhoc branch from f2f471e to 67e17f6 Compare April 20, 2018 15:07

aduth mentioned this pull request Apr 20, 2018

Add package: @wordpress/is-shallow-equal WordPress/packages#110

Merged

gziolo mentioned this pull request Apr 25, 2018

Editor: Make sure createInnerBlockList never updates when passed using context #6420

Merged

4 tasks

mcsf approved these changes Apr 27, 2018

View reviewed changes

youknowriad merged commit 1218df9 into master Apr 30, 2018

youknowriad deleted the add/purifyhoc branch April 30, 2018 09:05

youknowriad changed the title ~~Try adding a purify Higher Order Component~~ Try adding a pure Higher Order Component Apr 30, 2018

nerrad mentioned this pull request Jul 1, 2018

Fix potential flaw in pure Higher Order Component #7654

Closed

4 tasks

youknowriad mentioned this pull request Jul 23, 2018

export PureComponent #8120

Closed

3 tasks

Conversation

youknowriad commented Apr 20, 2018

Uh oh!

atimmer commented Apr 20, 2018

Uh oh!

youknowriad commented Apr 20, 2018

Uh oh!

gziolo commented Apr 20, 2018

Uh oh!

gziolo commented Apr 20, 2018

Uh oh!

mcsf commented Apr 20, 2018

Uh oh!

mcsf commented Apr 20, 2018

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Apr 25, 2018

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Apr 27, 2018

Uh oh!

aduth commented Apr 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gziolo commented Apr 25, 2018 •

edited

Loading