Adds missing dependencies to data-stores

[scinos]

Background

There are a few undeclared dependencies, found by running npx @yarnpkg/doctor:

➤ YN0000: ┌ /Users/sergio/src/automattic/wp-calypso/packages/data-stores/package.json
➤ YN0000: │ User scripts prefixed with "pre" or "post" (like "prepare") will not be called in sequence anymore; prefer calling prologues and epilogues explicitly
➤ YN0000: │ User scripts prefixed with "pre" or "post" (like "prepublish") will not be called in sequence anymore; prefer calling prologues and epilogues explicitly
➤ YN0000: │ /Users/sergio/src/automattic/wp-calypso/packages/data-stores/src/auth/test/flows.ts:15:1: Undeclared dependency on jest-fetch-mock-
➤ YN0000: │ /Users/sergio/src/automattic/wp-calypso/packages/data-stores/src/auth/test/flows.ts:16:1: Undeclared dependency on nock
➤ YN0000: │ /Users/sergio/src/automattic/wp-calypso/packages/data-stores/src/auth/test/flows.ts:17:1: Undeclared dependency on @testing-library/react
➤ YN0000: │ /Users/sergio/src/automattic/wp-calypso/packages/data-stores/package.json:39:19: Unmet transitive peer dependency on react@^16.8, via i18n-calypso@^5.0.0
➤ YN0000: └ Completed in 0.44s

Changes

• Adds missing dependencies to @testing-library/react, jest-fetch-mock and nock. Version ranges have been copied from ./package.json.
• Adds react and react-dom as devDependencies (they are peerDependencies of @testing-library/react). Version ranges have been copied from ./package.json as well.
• Add missing peerDependency to react, required by i18n-calypso.

The reasoning for declaring react twice is:

• React is required to develop this package, namely to run tests. As such, it must be a devDependency.
• React is required to be present when this package is imported, because i18n-calypso says so. But we don't want bring in our own copy, so we declare it as a peerDependency.

But to be honest, I'm not 100% sure of my reasoning. Please let me know if you think we should do something different.

Testing instructions

Run cd packages/data-stores && npx @yarnpkg/doctor and check the warnings about missing dependencies are gone

[matticbot]

Test live: https://calypso.live/?branch=fix/missing-deps-packages-data-stores

[matticbot]

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

[sirreal]

The added react dependencies seem odd to me. In theory, this package shouldn't depend on react as far as I'm aware. In practice, it'll probably always be used with react and this will be fine.

I'm surprised to see @testing-library/react. We aren't rendering or doing any lifecycle things in this package (I hope). A quick look suggests this is just re-exported from @testing-library/dom, so we could make that transitive dependency explicit and use it directly. I think that would be better (cc: @p-jackson @andrewserong via #40114)

React is required to be present when this package is imported, because i18n-calypso says so. But we don't want bring in our own copy, so we declare it as a peerDependency.

Darn! I wish we didn't have that dependency here 😞

[scinos]

Looks like we were depending on @testing-library/react only for their waitFor utility. I have replaced it with the package wait-for-expect that does the same and is much smaller.

[p-jackson]

Thank you! 🙇 wait-for-expect looks like a more appropriate package in this context; I only reached for @testing-library/react because that's something I already knew we had in node_modules. This is better for just API testing.

[alshakero]

@sirreal it seems React comes from i18n-calypso.

 Unmet transitive peer dependency on react@^16.8, via i18n-calypso@^5.0.0

[jsnajdr]

React is required to be present when this package is imported, because i18n-calypso says so.

i18n-calypso already declares react as its own peer dependency, so why declare it again in data-stores? The data-stores package doesn't use 'react' directly at all.

Moreover, data-stores doesn't really use React even indirectly: the i18n-calypso and @wordpress/data packages contain React bindings (hooks, HOCs, context providers, ...) for their functionality, but data-stores doesn't use these React bindings.

[scinos]

I think we either declare react as a peerDependency or as a dependency. It doesn't matter if data-stores doesn't use it: it is using a package that declares "if you want to use me, you need react". So by extension, anybody using data-stores needs react as well. If we don't declare it we risk things breaking if i18n-calypso starts to use react directly and not only its binings.

So if we must declare it, I think the best approach is to declare it as a peerDependency, otherwise we will be imposing our react version on any consumer of this package.

Edit: based on a quick test, yarn will complain about a missing transitive peerDependency but npm will not. So anybody can do npm install @automattic/data-stores, they won't get any warning, they won't get react in their node_modules. That can break as soon as a codepath expects react to be there.

Extra note: doing npm install @automattic/data-stores actually gives you node_modules/react, but only by accident because @wordpress/elements declares react as a regular dependency. Which can break the moment @wordpress/elements stops declaring react or uses a version incompatible with the version than i18n-calypso expects. More info in https://dev.to/arcanis/implicit-transitive-peer-dependencies-ed0

[jsnajdr]

The yarnpkg-doctor check seems to enforce that whenever one of package's dependencies declares a peer dependency, you need to copy all the peer declarations to the depending package, too. I don't understand why.

Peer dependency means that the package imports stuff from that package (e.g., webpack plugins import from the webpack package), but will never install that package themselves, in their private node_modules subdirectory. The parent tree is expected to provide the package, in one of the parent node_modules directories.

Why can't that work transitively? If wp-calypso depends on data-stores which depends on i18n-calypso, the react peer dependency in i18n-calypso says that one of the parents must provide react. If wp-calypso depends on react and installs it, everything is fine. The intermediate package, data-stores, doesn't need to declare the peer dependency again.

I'd expect Yarn to check the installed tree and fail in case when wp-calypso doesn't install react and the peer dependency is not satisfied. But why should it worry about data-stores?

I see one case where declaring the dependency on data-stores is important: if data-stores was a standalone repo:

// enter the data-stores directory
cd src/data-stores
// install its dependencies
yarn install

Now, the node_modules tree that has its root in src/data-stores is not valid: i18n-calypso requires react as a peer, but nobody installed it.

But data-stores is a workspace in a monorepo, not a standalone package. It doesn't have its own self-contained node_modules. And it's never used as a standalone app -- it's always a dependency of something.

[jsnajdr]

As I write in #45911 (comment), I think that adding react as a peer dependency of data-stores doesn't affect any scenario that can actually happen in practice. But the Yarn Doctor's advice follows the Hippocratic oath (do no harm), as it should, so let's follow it.

[scinos]

Why can't that work transitively?

If my tests are correct, that's just the way npm works. Peer dependencies are not "inherited". Yarn at least warns you about it 🤷

But data-stores is a workspace in a monorepo, not a standalone package.

It is also a package that we publish to NPM registry. That's the use case that breaks if the dependencies are not right.

[jsnajdr]

If my tests are correct, that's just the way npm works.

What were you testing for? That if your app depends on data-stores, and doesn't install react, then NPM won't detect an unsatisfied peer dependency in the transitive i18n-calypso package? Or something else?

It is also a package that we publish to NPM registry. That's the use case that breaks if the dependencies are not right.

That's fine, because the published package will be a dependency of something else, something that is supposed to install react. The only breaking case is doing npm install in the package's directory itself, without any parent package. But that never happens.

[scinos]

What were you testing for?

After a few more tests, this is the behaviour I see (using react-native@* which is also a missing peer dependency, so I don't need to hack @wordpress/element to not provide react)

#package.json
{
  "name": "test",
  "version": "1.0.0",
  "description": " ",
  "license": "ISC",
  "repository": {},
  "dependencies": {}
}

With NPM

# Installing for the first time
$ npm install @automattic/data-stores
npm WARN react-native-url-polyfill@1.2.0 requires a peer of react-native@* but none is installed. You must install peer dependencies yourself.
...

# No warning with clean node_modules (a bug in npm?)
$ rm -fr node_modules && npm install
added 76 packages from 76 contributors and audited 76 packages in 1.16s

# Warning when node_modules is up to date
$ npm install
npm WARN react-native-url-polyfill@1.2.0 requires a peer of react-native@* but none is installed. You must install peer dependencies yourself.
...

With Yarn

# Installing for the first time
$ yarn add @automattic/data-stores
warning "@automattic/data-stores > @wordpress/data > use-memo-one@1.1.1" has unmet peer dependency "react@^16.8.0".
warning "@automattic/data-stores > @wordpress/url > react-native-url-polyfill@1.2.0" has unmet peer dependency "react-native@*".
warning "@automattic/data-stores > @wordpress/data > @wordpress/compose > react-resize-aware@3.0.1" has unmet peer dependency "react@^16.8.0".
...

# Warning with clean node_modules
$ rm -fr node_modules && yarn
warning "@automattic/data-stores > @wordpress/data > use-memo-one@1.1.1" has unmet peer dependency "react@^16.8.0".
warning "@automattic/data-stores > @wordpress/url > react-native-url-polyfill@1.2.0" has unmet peer dependency "react-native@*".
warning "@automattic/data-stores > @wordpress/data > @wordpress/compose > react-resize-aware@3.0.1" has unmet peer dependency "react@^16.8.0".

# No warning when node_modules is up to date
$ yarn
success Already up-to-date.
...

Looks like both package managers will warn about missing peer dependencies in some cases, but looks like npm will omit some of them. I believe (but I haven't tested it) that it sees that someone else is depending on react and that's why it doesn't warn about it. If we don't declare react anywhere, consumers of this package will either get a warning, or no warning if (a compatible?)react is already in their tree.

I can't find any authoritative doc saying if a package should re-declare transitive peerDependencies or not, but given this comment from arcanis (emphasys mine), lead developer of yarn, I take it is as "highly recommended":

I'm a package author, and my package depends on another package which has a peer dependency on react. Since I don't want to provide this package I must indicate that my own package has a peer dependency on react

(from yarnpkg/berry#3)

[jsnajdr]

Thanks @scinos for the extra details 👍 Peer dependencies indeed look like a complex topic and all my attempts to understand it end up quickly in deep rabbit holes 🙁 There is, for example, this long conversation between NPM and Yarn maintainers about whether to automatically install them: npm/rfcs#43

Looks like both package managers will warn about missing peer dependencies in some cases, but looks like npm will omit some of them.

In your example, it's right for both package managers to warn, because the react-native package is really missing. And the peer dependency doesn't need to be transitive to trigger the warning -- even installing react-native-url-polyfill does trigger it.

NPM will not issue the warning only when package-lock.json is present. When installing from a lockfile, many npm install steps are short-circuited, and validating the peer dependencies seems to be one of them.

I can imagine at least one scenario where the NPM validation might fail us -- not warning although it should. Suppose that app package depends on component, which directly depends on react (normal dep) and also app dependsoni18n, which has reactas peer dependency. Afterappis npm-installed, there is anode_modules/reactpackage, because it's been hoisted up to the root folder. So, the peer dep of the sibling package is satisfied. Peer validation with an existingnode_modulestree will succeed. But it's unreliable -- thereactpackage can move tonode_modules/component/node_modules/reactat any time and theni18n` will no longer find it.

I suspect that this case might be somehow mitigated by re-declaring the peer deps, although I don't know why and don't want to research it at this moment.