Recategorize controller dependencies as devDependencies if only imports are messaging system-related types

[MajorLift]

Explanation

• Controller dependencies that are used via the messaging system should be listed under peerDependencies to ensure that we see warnings for incompatible versions.
• A peerDependencies controller that is not invoked directly at runtime shouldn't be included under dependencies. Any type imports will still resolve because we can assume the controller package will be installed.

References

Changelog

@metamask/accounts-controller

Fixed

• Add @metamask/snaps-controllers as a peer dependency in addition to a dev dependency (#3607)

@metamask/ens-controller

Fixed

• Recategorize @metamask/network-controller as a dev dependency and peer dependency (#3607)

@metamask/keyring-controller

Fixed

• Recategorize @metamask/preferences-controller as a dev dependency and peer dependency (#3607)

@metamask/permission-controller

Fixed

• Recategorize @metamask/approval-controller as a dev dependency and peer dependency (#3607)

@metamask/queued-request-controller

Fixed

• Recategorize @metamask/approval-controller, @metamask/network-controller, @metamask/selected-network-controller as dev dependencies (#3607)

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

[legobeat]

Related:

• Migrate accounts-controller to snaps repo #1699

[mcmire]

One thing but otherwise looks good!

[mcmire]

@legobeat Thanks for reminding me about that issue, I will take a closer look at it tomorrow.

[legobeat]

Would it be a sufficient compromise to add them as peerDependencies + devDependencies, and drop them as direct dependencies?

[Gudahtt]

That sounds like a better idea to me as well. This is better represented as a peerDependency because we're using it via the messaging system. A peerDependency is an expectation we have of the user of this package, which is the situation we're in here (we're expecting the user of this package to have a messaging system that includes a specific version of the KeyringController).

[MajorLift]

Thanks for explaining this! Applied here: b3be687

Also made the same changes to queued-request-controller (9b79604), and a few others (0e731b7) that we can revert if unnecessary.

queued-request-controller seems like an appropriate change though, since the controllers dependencies are only being used to import messaging types that are used in the middleware and not the controller itself.

[legobeat]

Great! Maybe we can go even further but makes sense to not do everything in one go.

[mcmire]

Upon a second review of this PR, does it make sense to do this? In all of these cases, dependencies are imported to use types from those dependencies. We want to make sure that those dependencies come along for the ride and aren't merely dev or peer dependencies, otherwise consumers that attempt to import these packages will get TypeScript compilation errors.

[MajorLift]

@mcmire That was my understanding going into this PR (packages from which we're only importing types can still be dependencies), which is why I initially recategorized keyring-controller and snaps-controllers from devDeps to deps.

[Gudahtt]

We want to make sure that those dependencies come along for the ride and aren't merely dev or peer dependencies

peerDependencies aren't suggestions, they're strict requirements. I know we can ignore them (with yarn at least), but it's still a totally appropriate way to express the demand here. And more than that, it would be incorrect to use a dependency here because then we'd see situations where types don't fail where they should (e.g. where the globally setup controller differs from one we have at type for, in a meaningful way).

We've been doing this a long time now. This is what this constraint is meant to enforce:

core/constraints.pro

Line 342 in 509e580

gen_enforced_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, 'peerDependencies') :-

[mcmire]

And more than that, it would be incorrect to use a dependency here because then we'd see situations where types don't fail where they should (e.g. where the globally setup controller differs from one we have at type for, in a meaningful way).

I'm not sure I follow. If the client is using a function or constructor from a library that expects an argument of a certain shape, and that shape comes from a dependency that the library loads, and the client passes an object that is of a different version of that shape because the library it uses is of a different version, TypeScript should fail.

We've been doing this a long time now

Have we? I know we've been using peerDependencies to enforce that if two controllers are talking to each other then the versions used should be compatible. But when it comes to types, the latest decision is that they should come along for the ride: #3578. Again, if those types aren't available, then TypeScript will fail to compile the code. Asking users to manually install dependencies to get types so they can compile their code seems non-ideal to me.

[Gudahtt]

If the client is using a function or constructor from a library that expects an argument of a certain shape, and that shape comes from a dependency that the library loads, and the client passes an object that is of a different version of that shape because the library it uses is of a different version, TypeScript should fail.

This would be true if metamask-controller.js was in TypeScript, but today it would result in no errors.

Generally though you're right, these peerDependency entries would be less critical if the extension used TypeScript, as we'd get the benefit of knowing the types matched up for any interactions. We still might find it valuable to use peerDependencies to represent expectations on what methods do, beyond just type compatibility, but maybe not in all cases.

I know we've been using peerDependencies to enforce that if two controllers are talking to each other then the versions used should be compatible.

That's exactly the situation here though, it's no different. The types aren't just being used for some unrelated purpose, they're used to represent the expected shape of the controller that the types come from.

But when it comes to types, the latest decision is that they should come along for the ride

We need to ensure the types are present if they're referenced in the types bundled with the package itself, otherwise those types would be invalid. But peerDependencies is an equally effective way of ensuring that they are present. Again, we have already being doing this an enforcing it via constraints. The same expectation of the types existing applies to communication between controllers.

We're not asking users to manually install additional code to get the types to work in this case. That's already required. These controllers won't work correctly if the controller referenced in the type isn't present, and integrated via the messenger system/controller constructor parameters.

[Gudahtt]

Oh, though I am noticing now that I have misremembered the constraint we have for controllers. It only checks that controller dependencies are set as peerDependencies, it ignores devDependencies. So we did have type references as dependencies, as you said. I don't recall why we would have done that.

Edit: While I don't recall why these controllers had each other as dependencies, it does make sense that we wouldn't have this constraint for devDependencies because some controllers are used only in tests. Though that pattern seems bad in itself (I don't think we have any good reason to use other controllers in unit tests), we of course wouldn't want to force ourselves to list it as a peerDependency just because it's in a test.

[Gudahtt]

To recap:

• If we're using another controller via the messaging system, it does seem important to list it under peerDependencies so that we get a warning if we're using a version that might not be compatible.
  • In the future, we can consider dropping this requirement and relying solely on types instead.
• If a controller is listed under peerDependencies and not invoked directly at runtime, I see no reason to include it under dependencies as well. The types will resolve because we can assume the package will be installed.

Does that make sense?

[mcmire]

I think so, yes. I'm on board.

[Gudahtt]

LGTM!