Element: Provide createHigherOrderComponent helper #5728

aduth · 2018-03-21T13:53:20Z

This pull request seeks to experiment with an alternative to @wordpress/element's getWrapperDisplayName function which is used to generate a display name for higher-order components. The use of this function is not always obvious, and can overcomplicate otherwise simple higher-order components by requiring the separate assignment of a class/function before then assigning displayName. The idea here is to eliminate consideration of the displayName with a generic helper for creating higher-order components, handling the assignment of the generated display name within the helper itself.

Implementation notes:

The code functionally works, though there are many other higher-order components we'd want to port if wanting to proceed, plus deprecating getWrapperDisplayName, or alternatively discouraging its use and leveraging it as the internal behavior of createHigherOrderComponent.

gziolo · 2018-03-21T17:30:26Z

element/index.js

+	return ( OriginalComponent ) => {
+		const EnhancedComponent = mapComponentToEnhancedComponent( OriginalComponent );
+		const { displayName = OriginalComponent.name || 'Component' } = OriginalComponent;
+		EnhancedComponent.displayName = `${ modifierName }(${ displayName })`;


We had upperFirst & camelCase on top of the modifier name to make sure it always looks like the name of the component. We may enforce it at the review time. Your call.

gziolo

This is what we exactly what we discussed some time ago. I like this new HOC helper. We should refactor also all the existing occurrences and deprecate getWrapperDisplayName. We should also adjust the existing test for getWrapperDisplayName to work with the HOC or provide something similar. I can help with that tomorrow.

gziolo · 2018-03-21T17:32:22Z

Yes, just wanted to say that I like how those HOCs evolve and make adding new functionalities easy to compose 💯

gziolo · 2018-03-22T09:30:37Z

@aduth - I added the following changes with d9bbcf9:

Covered createHigherOrderComponent with unit tests.
Added deprecation for getWrapperDisplayName .
Refactored all occurences of getWrapperDisplayName with createHigherOrderComponent.

I tried to make this PR small so in some cases we might perform more refactorings, but I wanted to avoid too much noise to make the review process easier.

gziolo · 2018-03-22T09:36:17Z

Something to think a bit more. I noticed that in many cases we define the same name for modifierName as the wrapping function. See:

export const withAlign = createHigherOrderComponent( ( BlockListBlock ) => {
	return ( props ) => {
		...
		return <BlockListBlock { ...props } wrapperProps={ wrapperProps } />;
	};
}, 'withAlign' );

I'm not sure if it is a good idea, but we might want to make the modifierName optional. In such case, we could take it directly from the wrapper function's name. Something to think about, as a further improvement. It might be also confusing and error-prone.

gziolo · 2018-03-22T09:37:02Z

element/index.js

+	return ( OriginalComponent ) => {
+		const EnhancedComponent = mapComponentToEnhancedComponent( OriginalComponent );
+		const { displayName = OriginalComponent.name || 'Component' } = OriginalComponent;
+		EnhancedComponent.displayName = `${ upperFirst( camelCase( modifierName ) ) }(${ displayName })`;


I replicated the old behavior upperFirst( camelCase( modifierName ) ) when porting old tests - feel free to update if you think it's not necessary.

gziolo · 2018-03-22T09:38:04Z

lib/client-assets.php

@@ -141,7 +141,7 @@ function gutenberg_register_scripts_and_styles() {
 	wp_register_script(
 		'wp-element',
 		gutenberg_url( 'element/build/index.js' ),
-		array( 'react', 'react-dom', 'react-dom-server' ),
+		array( 'react', 'react-dom', 'react-dom-server', 'wp-utils' ),


I hope it's temporary until we remove deprecated function :)

mcsf

Pending my observations, LGTM

mcsf · 2018-04-02T15:52:30Z

components/higher-order/with-state/index.js

-		return WrappedComponent;
-	};
+		};
+	} );


Missing 'withState' as modifierName.

(amended & pushed)

Thanks 👍

I don't see amend?

Oh. Maybe it got nuked in a subsequent rebase, @gziolo?

Yes, I did something wrong :(
Do you have this branch locally with your commit?

Yes, I did something wrong

My fault for rebasing your commit. ;)

Do you have this branch locally?

I do, the rebase is 0ae3d2 and the fix is e3a846.

Fixed with 66b2268, thanks!

mcsf · 2018-04-02T15:53:54Z

element/index.js

+ *
+ * @return {WPComponent} Component class with generated display name assigned.
+ */
+export function createHigherOrderComponent( mapComponentToEnhancedComponent, modifierName ) {


Since we don't attempt to infer modiferName, should there be a warning if the argument is missing?

I would leave it for reviewers for the time being. It seems to be a really great strategy 😃

In the next iteration, I would prefer to introduce the attempt infer this name and warn when it fails.

…erComponent

aduth · 2018-04-04T15:25:15Z

In such case, we could take it directly from the wrapper function's name. Something to think about, as a further improvement. It might be also confusing and error-prone.

Perhaps. I expect this wouldn't work so well for minified code (related: Automattic/wp-calypso#18650 ). Whether we care about this is another question, as we're not really using the name in runtime environments except maybe React DevTools.

gziolo · 2018-04-04T15:34:57Z

You can instrument uglify to preserve function names, but still it might not always work. It seems to be okay as it is in that context.

aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 21, 2018

aduth requested a review from gziolo March 21, 2018 13:53

gziolo reviewed Mar 21, 2018

View reviewed changes

gziolo approved these changes Mar 21, 2018

View reviewed changes

gziolo assigned gziolo and aduth Mar 22, 2018

gziolo changed the title ~~Try: Element: Provide createHigherOrderComponent helper~~ Element: Provide createHigherOrderComponent helper Mar 22, 2018

gziolo reviewed Mar 22, 2018

View reviewed changes

gziolo requested a review from a team March 30, 2018 09:53

gziolo force-pushed the add/create-hoc branch from d9bbcf9 to aaa0aad Compare March 30, 2018 09:55

gziolo added this to the 2.6 milestone Mar 30, 2018

mcsf approved these changes Apr 2, 2018

View reviewed changes

mcsf force-pushed the add/create-hoc branch from aaa0aad to 0ae3d2a Compare April 2, 2018 18:06

aduth and others added 2 commits April 3, 2018 11:26

Try: Element: Provide createHigherOrderComponent helper

4a2ab1c

Element: Depreacate getWrapperDisplayName in favor of createHigherOrd…

1614ae2

…erComponent

gziolo force-pushed the add/create-hoc branch from 0ae3d2a to 1614ae2 Compare April 3, 2018 09:28

gziolo merged commit 41fa06a into master Apr 3, 2018

gziolo deleted the add/create-hoc branch April 3, 2018 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Element: Provide createHigherOrderComponent helper #5728

Element: Provide createHigherOrderComponent helper #5728

aduth commented Mar 21, 2018

gziolo Mar 21, 2018

gziolo left a comment

gziolo commented Mar 21, 2018

gziolo commented Mar 22, 2018

gziolo commented Mar 22, 2018

gziolo Mar 22, 2018 •

edited

Loading

gziolo Mar 22, 2018

mcsf left a comment

mcsf Apr 2, 2018

mcsf Apr 2, 2018

gziolo Apr 3, 2018

aduth Apr 4, 2018

mcsf Apr 4, 2018

gziolo Apr 4, 2018

mcsf Apr 4, 2018

gziolo Apr 5, 2018 •

edited

Loading

mcsf Apr 2, 2018

gziolo Apr 3, 2018

aduth commented Apr 4, 2018

gziolo commented Apr 4, 2018

Element: Provide createHigherOrderComponent helper #5728

Element: Provide createHigherOrderComponent helper #5728

Conversation

aduth commented Mar 21, 2018

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

gziolo commented Mar 21, 2018

gziolo commented Mar 22, 2018

gziolo commented Mar 22, 2018

gziolo Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf 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

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

gziolo Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 4, 2018

gziolo commented Apr 4, 2018

gziolo Mar 22, 2018 •

edited

Loading

gziolo Apr 5, 2018 •

edited

Loading