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

Packages: Add a new compose package #7948

Merged
merged 6 commits into from
Jul 16, 2018
Merged

Packages: Add a new compose package #7948

merged 6 commits into from
Jul 16, 2018

Conversation

youknowriad
Copy link
Contributor

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.

@youknowriad youknowriad added the npm Packages Related to npm packages label Jul 13, 2018
@youknowriad youknowriad self-assigned this Jul 13, 2018
@youknowriad youknowriad requested review from gziolo and a team July 13, 2018 09:30
@youknowriad youknowriad force-pushed the add/compose-package branch from c66e582 to c25c23b Compare July 13, 2018 09:32
*/
import { flowRight } from 'lodash';

export { default as ifCondition } from './if-condition';
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

the same applies do devDependencies :)

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

Choose a reason for hiding this comment

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

Whitesepace issue.

@gziolo gziolo added the [Type] Breaking Change For PRs that introduce a change that will break existing functionality label Jul 13, 2018
@gziolo gziolo added this to the 3.3 milestone Jul 13, 2018
@gziolo
Copy link
Member

gziolo commented Jul 13, 2018

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
Copy link
Member

gziolo commented Jul 13, 2018

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.

return flowRight( ...args );
};

export const pure = createHigherOrderComponent( ( Wrapped ) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

@gziolo gziolo Jul 13, 2018

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

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.

@youknowriad youknowriad merged commit a6c36e5 into master Jul 16, 2018
@youknowriad youknowriad deleted the add/compose-package branch July 16, 2018 08:25
@@ -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...
Copy link
Member

Choose a reason for hiding this comment

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

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.


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

@aduth aduth Jul 16, 2018

Choose a reason for hiding this comment

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

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.

@gziolo gziolo added the [Package] Compose /packages/compose label Jul 16, 2018
@aduth aduth mentioned this pull request Aug 7, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Package] Compose /packages/compose [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants