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

Add constraint to ensure controllers are peerDependencies #1393

Merged
merged 2 commits into from
May 26, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 25, 2023

Description

A constraint has been added to ensure that each controller that a package depends on is also added as a peerDependency. This helps us ensure that controllers appear only once in the dependency tree on the client repositories.

Each controller is effectively a singleton because we only construct a single instance of each one. Typically they get added as dependencies only to get access to types in the controller package. If we don't add it as a peerDependency as well, it risks the versions getting out-of sync with what the client repository has installed.

Any missing peerDependency entries have been added to comply with this constraint.

Changes

None

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@Gudahtt Gudahtt requested a review from a team as a code owner May 25, 2023 23:09
@Gudahtt Gudahtt force-pushed the add-peer-dependency-constraint branch 3 times, most recently from 39149f0 to 7a3d5a6 Compare May 25, 2023 23:13
@Gudahtt Gudahtt mentioned this pull request May 26, 2023
constraints.pro Outdated Show resolved Hide resolved
Comment on lines +297 to +301
gen_enforced_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, 'peerDependencies') :-
workspace_has_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, 'dependencies'),
DependencyIdent \= '@metamask/base-controller',
DependencyIdent \= '@metamask/eth-keyring-controller',
is_controller(DependencyIdent).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're just interested in other workspaces, could we also write this as the following? Might remove the need for is_controller.

Suggested change
gen_enforced_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, 'peerDependencies') :-
workspace_has_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, 'dependencies'),
DependencyIdent \= '@metamask/base-controller',
DependencyIdent \= '@metamask/eth-keyring-controller',
is_controller(DependencyIdent).
gen_enforced_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, 'peerDependencies') :-
workspace_has_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, 'dependencies'),
DependencyIdent \= '@metamask/base-controller',
workspace_ident(_, DependencyIdent).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this would give us the wrong answer in two ways. We don't just care about workspaces here, we care about controllers. Controllers are singletons, regardless whether they live in this repo or not. But the other workspaces aren't. They're libraries that the clients can have multiple different versions of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gotcha. That's fair.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering keeping it just to controllers in this repo, and adding another constraint to prevent depending on other controllers. Ideally any that are depended on here would be here. I guess we can consider that separately though.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Gudahtt and others added 2 commits May 26, 2023 13:52
A constraint has been added to ensure that each controller that a
package depends on is also added as a `peerDependency`. This helps us
ensure that controllers appear only once in the dependency tree on the
client repositories.

Each controller is effectively a singleton because we only construct a
single instance of each one. Typically they get added as dependencies
only to get access to types in the controller package. If we don't add
it as a `peerDependency` as well, it risks the versions getting out-of
sync with what the client repository has installed.

Any missing `peerDependency` entries have been added to comply with
this constraint.
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
@Gudahtt Gudahtt force-pushed the add-peer-dependency-constraint branch from a8778d1 to 663dc30 Compare May 26, 2023 16:23
@Gudahtt Gudahtt merged commit db5f840 into main May 26, 2023
@Gudahtt Gudahtt deleted the add-peer-dependency-constraint branch May 26, 2023 16:30
brad-decker pushed a commit to dragana8/core that referenced this pull request Jun 1, 2023
)

* Add constraint to ensure controllers are peerDependencies

A constraint has been added to ensure that each controller that a
package depends on is also added as a `peerDependency`. This helps us
ensure that controllers appear only once in the dependency tree on the
client repositories.

Each controller is effectively a singleton because we only construct a
single instance of each one. Typically they get added as dependencies
only to get access to types in the controller package. If we don't add
it as a `peerDependency` as well, it risks the versions getting out-of
sync with what the client repository has installed.

Any missing `peerDependency` entries have been added to comply with
this constraint.

* Fix misleading local variable name

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* Add constraint to ensure controllers are peerDependencies

A constraint has been added to ensure that each controller that a
package depends on is also added as a `peerDependency`. This helps us
ensure that controllers appear only once in the dependency tree on the
client repositories.

Each controller is effectively a singleton because we only construct a
single instance of each one. Typically they get added as dependencies
only to get access to types in the controller package. If we don't add
it as a `peerDependency` as well, it risks the versions getting out-of
sync with what the client repository has installed.

Any missing `peerDependency` entries have been added to comply with
this constraint.

* Fix misleading local variable name

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* Add constraint to ensure controllers are peerDependencies

A constraint has been added to ensure that each controller that a
package depends on is also added as a `peerDependency`. This helps us
ensure that controllers appear only once in the dependency tree on the
client repositories.

Each controller is effectively a singleton because we only construct a
single instance of each one. Typically they get added as dependencies
only to get access to types in the controller package. If we don't add
it as a `peerDependency` as well, it risks the versions getting out-of
sync with what the client repository has installed.

Any missing `peerDependency` entries have been added to comply with
this constraint.

* Fix misleading local variable name

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants