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

Peer Dependencies missing in NPM packages #37264

Closed
jonshipman opened this issue Dec 9, 2021 · 17 comments · Fixed by #37578
Closed

Peer Dependencies missing in NPM packages #37264

jonshipman opened this issue Dec 9, 2021 · 17 comments · Fixed by #37578
Labels
Developer Experience Ideas about improving block and theme developer experience npm Packages Related to npm packages [Status] In Progress Tracking issues with work in progress

Comments

@jonshipman
Copy link

jonshipman commented Dec 9, 2021

Description

Yarn 2 (Cherry) implements Plug'n'Play versus the node_modules node linker. One major issue that I have noticed is many packages inside gutenberg are missing peerDependencies on several libraries. This is frequently react, react-dom, or redux. In a traditional npm or yarn 1 environment this is a non-issue as the library will be present in the node_modules folder.

Potential solutions:

  1. Add the missing peerDependencies as dependencies.
  2. Add the missing peerDependencies as dependencies to the calling package.
  3. Add the missing peerDependencies as peerDependencies to the calling package.

Below is a small list provided as an example listed from running yarn when pnp is configured as a nodeLinker.

@wordpress/block-editor@npm:8.0.9 doesn't provide react (p3ee78), requested by @react-spring/web
@wordpress/block-editor@npm:8.0.9 doesn't provide react (p97514), requested by react-autosize-textarea
@wordpress/block-editor@npm:8.0.9 doesn't provide react (p9f942), requested by react-easy-crop
@wordpress/block-editor@npm:8.0.9 doesn't provide react-dom (p1040a), requested by @react-spring/web
@wordpress/block-editor@npm:8.0.9 doesn't provide react-dom (p94024), requested by react-autosize-textarea
@wordpress/block-editor@npm:8.0.9 doesn't provide react-dom (p10834), requested by react-easy-crop
@wordpress/block-editor@npm:8.0.9 doesn't provide reakit-utils (pa584b), requested by @wordpress/components
@wordpress/block-editor@npm:8.0.9 doesn't provide redux (p3a664), requested by @wordpress/data
@wordpress/block-library@npm:6.0.13 doesn't provide reakit-utils (p79b87), requested by @wordpress/components
@wordpress/block-library@npm:6.0.13 doesn't provide redux (p1bccb), requested by @wordpress/data
@wordpress/blocks@npm:11.1.4 doesn't provide redux (pa0e23), requested by @wordpress/data
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p4d03f), requested by react-resize-aware
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p09209), requested by @emotion/react
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (pccf1b), requested by @emotion/styled
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p4a54a), requested by downshift
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p6e898), requested by framer-motion
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p2d5cb), requested by re-resizable
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p510ae), requested by react-colorful
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p56d3a), requested by react-dates
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p8cd80), requested by react-use-gesture
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react (p6ed4f), requested by reakit
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react-dom (p1f814), requested by framer-motion
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react-dom (p5af1b), requested by re-resizable
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react-dom (p79453), requested by react-colorful
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react-dom (p8c65d), requested by react-dates
@wordpress/components@npm:19.1.3 [6858d] doesn't provide react-dom (p157f9), requested by reakit
@wordpress/compose@npm:5.0.6 doesn't provide react (p1c8c4), requested by react-resize-aware
@wordpress/compose@npm:5.0.6 doesn't provide react (p84d9f), requested by use-memo-one
@wordpress/core-data@npm:4.0.8 doesn't provide redux (p03661), requested by @wordpress/data
@wordpress/data-controls@npm:2.2.7 doesn't provide redux (pcfba6), requested by @wordpress/data
@wordpress/data@npm:6.1.4 [25de6] doesn't provide react (p88089), requested by use-memo-one
@wordpress/edit-post@npm:5.0.15 doesn't provide reakit-utils (p0b932), requested by @wordpress/components
@wordpress/edit-post@npm:5.0.15 doesn't provide redux (pe2188), requested by @wordpress/data
@wordpress/editor@npm:12.0.12 doesn't provide react (p643f1), requested by react-autosize-textarea
@wordpress/editor@npm:12.0.12 doesn't provide react-dom (pd46af), requested by react-autosize-textarea
@wordpress/editor@npm:12.0.12 doesn't provide reakit-utils (pe32c6), requested by @wordpress/components
@wordpress/editor@npm:12.0.12 doesn't provide redux (p329d3), requested by @wordpress/data
@wordpress/eslint-plugin@npm:9.3.0 [ed588] doesn't provide @babel/core (p4f0ab), requested by @babel/eslint-parser
@wordpress/interface@npm:4.1.11 doesn't provide reakit-utils (p93f29), requested by @wordpress/components
@wordpress/interface@npm:4.1.11 doesn't provide redux (p00a6b), requested by @wordpress/data
@wordpress/jest-preset-default@npm:7.1.3 [ed588] doesn't provide @babel/core (pff225), requested by babel-jest
@wordpress/jest-preset-default@npm:7.1.3 [ed588] doesn't provide react (pcb47d), requested by @wojtekmaj/enzyme-adapter-react-17
@wordpress/jest-preset-default@npm:7.1.3 [ed588] doesn't provide react-dom (p39e2c), requested by @wojtekmaj/enzyme-adapter-react-17
@wordpress/keyboard-shortcuts@npm:3.0.6 doesn't provide redux (p27c4e), requested by @wordpress/data
@wordpress/notices@npm:3.2.7 doesn't provide redux (p8d2e8), requested by @wordpress/data
@wordpress/postcss-plugins-preset@npm:3.2.5 doesn't provide postcss (pf46d7), requested by autoprefixer
@wordpress/reusable-blocks@npm:3.0.15 doesn't provide reakit-utils (paf1cf), requested by @wordpress/components
@wordpress/reusable-blocks@npm:3.0.15 doesn't provide redux (p04ebd), requested by @wordpress/data
@wordpress/rich-text@npm:5.0.6 doesn't provide redux (p57a49), requested by @wordpress/data
@wordpress/scripts@npm:19.2.2 doesn't provide @babel/core (p1cb89), requested by babel-jest
@wordpress/scripts@npm:19.2.2 doesn't provide @babel/core (p74092), requested by babel-loader
@wordpress/server-side-render@npm:3.0.13 doesn't provide reakit-utils (pb1fdf), requested by @wordpress/components
@wordpress/server-side-render@npm:3.0.13 doesn't provide redux (p6451e), requested by @wordpress/data
@wordpress/viewport@npm:4.0.6 doesn't provide redux (pd3869), requested by @wordpress/data

This list is not extensive. As an example, line 1 could be fixed by adding 'react' to the peerDependencies of the @wordpress/block-editor package. Alternatively, since react is a dependency of @wordpress/element - all the packages could list that as a peerDependency that are missing react or react-dom.

Step-by-step reproduction instructions

Create a new yarn cherry project and require any of the above listed components (@wordpress/components for example).

  1. Create new directory.
  2. yarn init -2 -y
  3. yarn install
  4. yarn add -D @wordpress/components
  5. yarn

The screen should relay which dependencies are missing for @wordpress/components. These warnings prevent building and start scripts from executing.

For more details on Yarn and its installation, visit https://yarnpkg.com/getting-started/install

Screenshots, screen recording, code snippet

No response

Environment info

yarn@3.1.1
node@16.2.0

This issue specifically addresses the NPM packages inside gutenberg/packages.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@gziolo gziolo added Developer Experience Ideas about improving block and theme developer experience npm Packages Related to npm packages labels Dec 9, 2021
@gziolo
Copy link
Member

gziolo commented Dec 13, 2021

Some of the issues listed are surprising.

I can see why Yarn complains about Redux:

"peerDependencies": {
"redux": "^4.1.0"
},

It's listed as a peer dependency and I don't remember why it isn't set as a regular dependency. It feels like it should be a regular dependency.

React and React DOM shouldn't be listed there because they are dependencies of @wordpress/element that is listed as a dependency in the packages that complain:

"react": "^17.0.1",
"react-dom": "^17.0.1"

Unless the 3rd party libraries define different ranges for React and React DOM.

I need to investigate @babel/core - that one might be a valid concern.

@gziolo
Copy link
Member

gziolo commented Dec 13, 2021

I need to investigate @babel/core - that one might be a valid concern.

Opened PR that should address the issue for @babel/core.

@gziolo
Copy link
Member

gziolo commented Dec 13, 2021

I can see why Yarn complains about Redux:

"peerDependencies": {
"redux": "^4.1.0"
},

It's listed as a peer dependency and I don't remember why it isn't set as a regular dependency. It feels like it should be a regular dependency.

@youknowriad or @jsnajdr, do you know why we have redux set as a peer dependency here? It's set as a regular dependency in @wordpress/redux-routine:

We don't even define redux in the top-level package.json so I guess something is wrong here.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Dec 13, 2021
@youknowriad
Copy link
Contributor

@youknowriad or @jsnajdr, do you know why we have redux set as a peer dependency here? It's set as a regular dependency in @wordpress/redux-routine:

I guess the reasoning is that when you use the data module, you don't necessarily use redux-based stores but that reasoning is a bit flawed to be honest as we'll be using redux in 80% of the time (if not more). Anyway, this was also one of the issues (among other missing dependencies) that were highlighted by attempting to move to pnpm in #37324 :)

@jsnajdr
Copy link
Member

jsnajdr commented Dec 13, 2021

This is a mistake introduced in #21313. Look at this discussion thread:

https://github.com/WordPress/gutenberg/pull/21313/files/2b21f859560b1c5059da220c05d10ca262cfe20e#r657773258

It was proposed to set redux as peer in redux-routine, but the change was done in data instead.

@wordpress/data calls createStore and applyMiddleware from redux, it's an implementation detail of the package, a clear regular dep.

In redux-routine, redux is imported solely to declare middleware TS types. And it's a plugin. Peer dep is appropriate.

@gziolo
Copy link
Member

gziolo commented Dec 14, 2021

@wordpress/data calls createStore and applyMiddleware from redux, it's an implementation detail of the package, a clear regular dep.

In redux-routine, redux is imported solely to declare middleware TS types. And it's a plugin. Peer dep is appropriate.

Thank you, that explains everything. It's an obvious mistake that needs to be addressed according to your explanation 👍

@gziolo
Copy link
Member

gziolo commented Dec 14, 2021

I opened #37364 to fix the issues with redux configuration in package.json files.

@gziolo
Copy link
Member

gziolo commented Dec 14, 2021

@diegohaz or @ciampo, do you know why reakit-utils is listed as a peer dependency in @wordpress/components:

"peerDependencies": {
"reakit-utils": "^0.15.1"
},

I don't see any reference in the code other than in code comments next to TypeScript types.

@diegohaz
Copy link
Member

@diegohaz or @ciampo, do you know why reakit-utils is listed as a peer dependency in @wordpress/components:

"peerDependencies": {
"reakit-utils": "^0.15.1"
},

I don't see any reference in the code other than in code comments next to TypeScript types.

I think we've forgotten to remove it after a refactor. We shouldn't depend on that package.

@gziolo
Copy link
Member

gziolo commented Dec 14, 2021

I opened #37369 to fix the issue with reakit-utils.

@jonshipman, I think I addressed most of the issues from the list you shared. The biggest remaining question is whether we can do anything about reports for react and react-dom. In theory, they all should be satisfied through @wordpress/element so I would argue that the validator is too strict. What do you think?

@noahtallen
Copy link
Member

Thanks for working on this; I also should have raised an issue several months ago! As a work-around, we have a significant number of overrides in our .yarnrc.yml file here: https://github.com/Automattic/wp-calypso/blob/trunk/.yarnrc.yml

@gziolo
Copy link
Member

gziolo commented Dec 17, 2021

Thanks for working on this; I also should have raised an issue several months ago! As a work-around, we have a significant number of overrides in our .yarnrc.yml file here: https://github.com/Automattic/wp-calypso/blob/trunk/.yarnrc.yml

Thanks for sharing @noahtallen. It pretty much aligns with what @jonshipman reported. I expect that most of the issues are resolved with all the recent changes included. The only thing that bothers me here is React dependencies. It raises the question of whether we should declare it as a peer dependency using "^17.0.0" range in every package that uses any external React library and accesses it directly. I must admit that those checks are very strict because the dependency is satisfied through @wordpress/element. I'm sure I miss something obvious here.

@ZebulanStanphill
Copy link
Member

Logically, any package accessed directly by the files of a package should be declared as a dependency of that package, right? By definition, accessing exports from react means that react is a top-level dependency. If we were truly relying solely on @wordpress/element, we would be accessing the exports through that package (which could re-export them). But in many cases, we're not. Therefore, we ought to add react to the list of explicit dependencies, because that's how we're using it: explicitly.

@gziolo
Copy link
Member

gziolo commented Dec 20, 2021

@ZebulanStanphill, I think that reasoning makes sense. It's a bit unfortunate that we have to track down peer dependencies of dependencies and repeat them in our packages, but maybe it's worth the hassle.

I published to npm a development version of npm packages. It should allow us to test all the latest improvements applied to WordPress packages. A good example would be installing @wordpress/block-editor package with the next dist-tag to verify whether it still complains about the missing redux of reakit-utils:

npm install --save @wordpress/block-editor@next

The stable version should be available in the second half of January.

@jsnajdr
Copy link
Member

jsnajdr commented Dec 20, 2021

Logically, any package accessed directly by the files of a package should be declared as a dependency of that package, right?

Yes, there are some consistency rules that all packages should satisfy.

If a package does import from 'react' directly, it must have react in its own dependencies. It's not enough to depend on @wordpress/element which in turn depends on react, because then react can be installed in a directory like:

./node_modules/@wordpress/element/node_modules/react

And the ./component.js module won't find it there. That package is findable only by modules inside ./node_modules/@wordpress/element

Depending on @wordpress/element only also won't satisfy a react peer dependency of a 3rd party package:

"dependencies": {
  "@wordpress/element": "*",
  "react-component-that-declares-a-peer": "*"
}

For the same reason as above, react-component-that-declares-a-peer is not really guaranteed to have react available.

The second rule, enforced especially by newer versions of Yarn, is that when a package depends on a package with a peer dependency, it must either satisfy it, or redeclare it explicitly itself. You must do either this:

"dependencies": {
  "react-component-that-declares-a-peer": "*",
  "react": "*" // satisfy the dep yourself
}

or this:

"dependencies": {
  "react-component-that-declares-a-peer": "*"
},
"peerDepedencies": {
  "react": "*" // forward the dep to parent
}

but this alone won't do:

"dependencies": {
  "react-component-that-declares-a-peer": "*"
}

The argument why it's not sufficient is rather complex and is described by the Yarn maintainer in this post.

@gziolo
Copy link
Member

gziolo commented Dec 20, 2021

@jsnajdr, thanks for a detailed summary of how Yarn approaches the problem. It's a bit unfortunate that we need to put extra care here, but it's manageable. Let's put react and react-dom as peer dependencies in all places where Yarn compalains 👍🏻

@gziolo
Copy link
Member

gziolo commented Dec 22, 2021

Hopefully, the last PR #37578 for react and react-dom is available now for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience npm Packages Related to npm packages [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants