Categorize type-only and controller-messaged deps correctly

[mcmire]

Explanation

There are two special kinds of dependencies that have been frequently miscategorized:

• Type-only dependencies: These are dependencies from which no runtime code is used, but only TypeScript types. Since consumers need these types in order to write TypeScript code, these should be listed under dependencies (not devDependencies).
• Controller-messaged dependencies. Say that controller A talks to controller B. Controller B in this case needs to be a dependency of A. Furthermore, a project that wants to use controller A needs to instantiate B before A. Because of the intercommunication between these controllers, in order to prevent runtime errors, the version of controller B that A relies on must match the version of B that the project relies on. To enforce this, B must be listed under A's peerDependencies, even if it is also listed under its dependencies.

This commit corrects dependencies for each package in the monorepo such that they follow the rules above. In rare cases dependencies that were completely used in production code were listed under devDependencies. These have been corrected as well.

References

Fixes #2062, and related (but unfiled) problems.

Changelog

(Updated in PR)

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[Gudahtt]

Since consumers need these types in order to write TypeScript code, these should be listed under dependencies (not devDependencies).

This seems like a misunderstanding. It's not typically the case that consumers would need the same type dependencies to be in the dependency tree in order to use a package's types.

This is only the case when another packages types are exported directly. If that happens, that dependency's types will also end up directly referenced in the resulting types. Any project using this package would need to have the package the types come from as well, or it would be an invalid reference.

But that's not true when the types aren't exported. If they're just used internally by the package, they don't exist at all for package consumers. This is more commonly the case, and in general I think we should avoid exporting third-party types so that our packages can have fewer dependencies.

You can see some examples of what I mean in this diff. If you build all packages and look at packages/accounts-controller/dist/types/AccountsController.d.ts, you can see this:

import type { SnapStateChange } from '@metamask/snaps-controllers';
...
export type AllowedEvents = SnapStateChange | KeyringControllerStateChangeEvent;

That type comes from @metamask/snaps-controllers and is exported directly. We'd need to ensure that @metamask/snaps-controllers is present for users of this package in order for these types to remain valid.

However, if you look at the two packages you've moved to dependencies in assets-controllers (@metamask/keyring-api and @types/lodash), neither of them appear in the built types. They are only needed at build-time, they aren't used at all by consumers of this package.

[Gudahtt]

Additionally, in general we should never have a package listed as both a peerDependency and a dependency. If something is a peerDependency, it means that we expect the user of the package to bring their own copy. It would be a contradiction to list it as a dependency in that case; we'd be asking them to bring their own copy, but we'd have ours as well. The whole point of a peerDependency is that we don't know which version is correct to use. We need to rely upon the user of the package to be responsible for having the correct version.

Every peerDependency should be listed as a devDependency so that the package is present here for our tests to run, but it should never be a dependency.

[mcmire]

Thanks for the feedback! I have to think about this a bit more. Moving to draft in the meantime.

[mcmire]

I've thought about it and your suggestion makes sense. It sounds like I need to adjust the constraints to disallow a package from appearing in both dependencies and peerDependencies, and perhaps to add some documentation around how to represent controller dependencies effectively.

I'll return to this after we've upgraded to Yarn v4 and converted the constraints to JavaScript, because I think it'll be easier to modify the constraints then.