Packages: Add a new compose package by youknowriad · Pull Request #7948 · WordPress/gutenberg

youknowriad · 2018-07-13T09:30:09Z

This PR extracts some basic Higher-Order Components from the "element" and "components" modules into a "compose" package.

The idea is:

Provide an equivalent of recompose for WordPress element.
Leave the components package for UI related components and HoCs making it the pattern library of WordPress.
Fix the cycle dependency we have in viewport Where viewport depends on components and components depend on viewport.

Testing intructions

Make sure the tests pass without regression
Check that the deprecation messages are shown properly.

…pose package

gziolo · 2018-07-13T09:38:15Z

packages/compose/src/index.js

+ */
+import { flowRight } from 'lodash';
+
+export { default as ifCondition } from './if-condition';


What about createHigherOrderComponent? In my opinion, it fits here as well. All HOCs are created using it. It isn't very useful on its own inside element package.

gziolo · 2018-07-13T09:39:20Z

packages/compose/package.json

+	"dependencies": {
+		"@wordpress/element": "file:../element",
+		"@wordpress/is-shallow-equal": "file:packages/is-shallow-equal",
+		"lodash": "4.17.10"


Can we use range for all dependencies, e.x.:
"lodash": "^4.17.10"
We use that in all other packages.

the same applies do devDependencies :)

gziolo · 2018-07-13T09:39:58Z

packages/element/src/index.js

- * WordPress dependencies
- */
-import isShallowEqual from '@wordpress/is-shallow-equal';
+import { 	isString } from 'lodash';


Whitesepace issue.

gziolo · 2018-07-13T09:47:51Z

I think you need to add @wordpress/compose to the main package.json file as a local dependency to make tests pass. Otherwise, Jest doesn't know how to load it.

gziolo · 2018-07-13T09:51:21Z

This is looking great, thanks for spinning up this PR. I was advocating for it for quite some time and I'm super excited it is happening 💯
As noted earlier, some unit tests are failing, which might be only because of package.json changes required. I didn't look at failing e2e tests.

gziolo · 2018-07-13T09:55:35Z

packages/element/src/deprecated.js

+	return flowRight( ...args );
+};
+
+export const pure = createHigherOrderComponent( ( Wrapped ) => {


Yeah, probably makes the most sense to keep those two items duplicated until they get removed 👍

gziolo · 2018-07-13T09:58:50Z

packages/compose/package.json

+	"module": "build-module/index.js",
+	"dependencies": {
+		"@wordpress/element": "file:../element",
+		"@wordpress/is-shallow-equal": "file:packages/is-shallow-equal",


npm ERR! Could not install from "packages/compose/packages/is-shallow-equal" as it does not contain a package.json file.

It should be:
"@wordpress/is-shallow-equal": "file:../is-shallow-equal",

Path needs to be relative, at least this is how Lerna does it. I mirrored their configuration.

gziolo · 2018-07-13T10:40:20Z

packages/compose/package.json

 		"lodash": "^4.17.10"
 	},
 	"devDependencies": {
+		"@wordpress/jest-console": "file:packages/jest-console",


I don't think it should be included here. We never import it explicitly in the test file.

gziolo

Great work on this PR. I tested it locally and it works perfectly fine. I could use deprecated methods and warnings were printed on the JS console.

I added a tiny change to package.json which allows ranges for dev dependencies to align with other packages and general approach we took.

aduth · 2018-07-16T13:07:40Z

packages/compose/README.md

@@ -0,0 +1,27 @@
+# @wordpress/compose
+
+The `compose` package is a collection of handy [Higher Order Components](https://facebook.github.io/react/docs/higher-order-components.html) you can use to wrap your WordPress components and provide some basic features like: State, instanceId, pure...


I think we should use the documentation to make it extremely clear the distinction we have been modules like compose, components, and elements (and data, viewport, etc), as even internally we've not been entirely consistent with what the approach is. With this being an effort to standardize the approach, we could do well to explain it thoroughly to avoid ambiguity.

aduth · 2018-07-16T15:20:10Z

components/deprecated.js

+
+export default mapValues( deprecatedFunctions, ( deprecatedFunction, key ) => {
+	return ( ...args ) => {
+		deprecated( 'wp.components.' + key, {


Historically our deprecated ESLint rule has stumbled spectacularly on any sort of cleverness. Did we check to see how this works (re: the concatenation)?

Also, with "complications" like the need to remove exports from index.js which is not entirely obvious by this deprecation call alone, it would be good to have included a comment noting said needs to the future maintainer.

youknowriad added the npm Packages Related to npm packages label Jul 13, 2018

youknowriad self-assigned this Jul 13, 2018

youknowriad requested review from a team and gziolo July 13, 2018 09:30

Compose: Extract UI-less Higher order components into a wordpress/com…

c25c23b

…pose package

youknowriad force-pushed the add/compose-package branch from c66e582 to c25c23b Compare July 13, 2018 09:32

gziolo reviewed Jul 13, 2018

View reviewed changes

gziolo added the [Type] Breaking Change For PRs that introduce a change that will break existing functionality label Jul 13, 2018

gziolo added this to the 3.3 milestone Jul 13, 2018

Use a range version for lodash

641f98a

gziolo reviewed Jul 13, 2018

View reviewed changes

youknowriad added 2 commits July 13, 2018 11:04

Move createHigherOrderComponent to @wordpress/compose

318ac15

Fix unit tests

11d939a

gziolo reviewed Jul 13, 2018

View reviewed changes

Remove useless dev dependency

fd299fe

ajitbohra mentioned this pull request Jul 13, 2018

Implemented Gallery Drag and Drop Reordering #7946

Closed

4 tasks

Compose: Use ranges for dev dependencies

6c46a47

gziolo approved these changes Jul 16, 2018

View reviewed changes

youknowriad merged commit a6c36e5 into master Jul 16, 2018

youknowriad deleted the add/compose-package branch July 16, 2018 08:25

Shelob9 mentioned this pull request Jul 16, 2018

Packages: Publish all modules as independent npm packages #3955

Closed

19 tasks

aduth mentioned this pull request Jul 16, 2018

Components: Port Popover to use withViewportMatch #5316

Closed

aduth reviewed Jul 16, 2018

View reviewed changes

gziolo added the [Package] Compose /packages/compose label Jul 16, 2018

aduth mentioned this pull request Aug 7, 2018

Block settings extensibility #7895

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packages: Add a new compose package#7948

Packages: Add a new compose package#7948
youknowriad merged 6 commits intomasterfrom
add/compose-package

youknowriad commented Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018

Uh oh!

gziolo commented Jul 13, 2018

Uh oh!

gziolo commented Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018 •

edited

Loading

Uh oh!

gziolo Jul 13, 2018

Uh oh!

gziolo left a comment

Uh oh!

aduth Jul 16, 2018

Uh oh!

aduth Jul 16, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,27 @@
		# @wordpress/compose

		The `compose` package is a collection of handy [Higher Order Components](https://facebook.github.io/react/docs/higher-order-components.html) you can use to wrap your WordPress components and provide some basic features like: State, instanceId, pure...

Conversation

youknowriad commented Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo commented Jul 13, 2018

Uh oh!

gziolo commented Jul 13, 2018

Uh oh!

gziolo Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

aduth Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gziolo Jul 13, 2018 •

edited

Loading

aduth Jul 16, 2018 •

edited

Loading