Missing "@types/react" dependency

[slavafomin]

Hello!

Thank you for this great library!

You are referencing react in public type declarations of your components, however, the @types/react dependency is missing from the manifest of the @material-ui/core package.

This leads to type-checking errors, because the types for react package couldn't be resolved from your declarations:

ERROR in /node_modules/.registry.npmjs.org/@material-ui/core/4.3.3_react-dom@16.9.0+react@16.9.0/node_modules/@material-ui/core/Avatar/Avatar.d.ts

ERROR in /node_modules/.registry.npmjs.org/@material-ui/core/4.3.3_react-dom@16.9.0+react@16.9.0/node_modules/@material-ui/core/Avatar/Avatar.d.ts(1,24):
TS7016: Could not find a declaration file for module 'react'. '/node_modules/.registry.npmjs.org/react/16.9.0/node_modules/react/index.js' implicitly has an 'any' type.

This happens in projects, which use more strict dependency management tools, like rush / pnpm, because they block access to implicit dependencies (like in your case).

In order to fix this, I would suggest to add @types/react and @types/react-dom packages to the peerDependencies section of the @material-ui/core package manifest.

Thanks!

[oliviertassinari]

In order to fix this, I would suggest to add @types/react and @types/react-dom packages to the peerDependencies section of the @material-ui/core package manifest.

Interesting, I see that it's the approach used by Apollo but that Antd uses the same approach as us. I think that we should pick a tradeoff that best serves npm and yarn users, rush / pnpm have a marginal user base. We can better support them at the condition it doesn't harm the majority.

cc @eps1lon, @merceyz, @pelotom.

[slavafomin]

Declaration files are the same code modules as anything else and if you use some third-party types in them and expose them in your own types then those types must be specified as dependencies for your package. It is as simple as that.

The way it works in npm / yarn projects is just a happy coincidence due to the simplified way of how those package managers install dependencies.

There are no multiple ways to handle this, there is only a one way, correct way — explicitly specify all the dependencies in the manifest.

pnpm is very good at detecting such issues and it's behavior is much more precise than of npm (which just closes it's eyes on many dependency issues).

And there is no harm in explicitly specifying the dependencies, it won't break anything :)

[oliviertassinari]

@slavafomin Maybe we would have these two packages as dependencies like we do with @types/react-transition-group (but not peer dependencies)?

The way it works in npm / yarn projects is just a happy coincidence due to the simplified way of how those package managers install dependencies.

I wouldn't say a coincidence, I believe it was taken into account in the current tradeoff. We don't use a peer dependency which avoids a spoil of the console for JavaScript users and we don't explicit the dependency version to avoid duplication or mismatch of versions of the definitions. But maybe there is a better approach, I don't know.

[pelotom]

In whatever way we depend on react itself (currently peerDependencies?) we should depend in the same way on @types/react. This is my understanding of the best way to manage type dependencies.

[eps1lon]

This is where peerDependenciesMeta (i.e. optional peer dependencies) comes in. yarn already implements this, npm as well (though not the version bundled with node yet) and pnpm implements this as well.

I'll put together a PR that we'll merge once npm 6.11 is bundled with node. Basically once nodejs/node#29273 is released to node LTS (10).