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.

Sign up for GitHub

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

Use static versions for interdependencies #1623

Merged
merged 5 commits into from
Aug 25, 2023
Merged

Use static versions for interdependencies #1623

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8dc7199
Use static versions for interdependencies
mcmire Aug 5, 2023
c8f94e6
Merge branch 'main' into make-yarn-link-work-again
mcmire Aug 24, 2023
69ed88f
Merge branch 'main' into make-yarn-link-work-again
mcmire Aug 24, 2023
811e5ea
Rewrite section on testing changes to include local
mcmire Aug 24, 2023
cf79ca4
Merge branch 'main' into make-yarn-link-work-again
mcmire Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions constraints.pro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,14 @@ gen_enforced_dependency(WorkspaceCwd, DependencyIdent, 'a range optionally start
workspace_has_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, DependencyType),
\+ is_valid_version_range(DependencyRange).

% All references to a workspace package must be up to date with the current
% version of that package.
gen_enforced_dependency(WorkspaceCwd, DependencyIdent, CorrectDependencyRange, DependencyType) :-
workspace_has_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, DependencyType),
workspace_ident(OtherWorkspaceCwd, DependencyIdent),
workspace_version(OtherWorkspaceCwd, OtherWorkspaceVersion),
atomic_list_concat(['^', OtherWorkspaceVersion], CorrectDependencyRange).

% All dependency ranges for a package must be synchronized across the monorepo
% (the least version range wins), regardless of which "*dependencies" field
% where the package appears.
Expand Down
4 changes: 2 additions & 2 deletions packages/address-book-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/utils": "^6.2.0"
},
"devDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion packages/announcement-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^"
"@metamask/base-controller": "^3.2.0"
Copy link
Contributor

@legobeat legobeat Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Has it been considered to use unpublished placeholder versions here? Like ^3.2.0-next. The published versions would of course have their package.json aligned with published versions. This would remove the risk of accidentally resolving from registry while keeping the ease of override in this PR, I think?

It's a pattern I recall seeing from time to time.

Copy link
Contributor Author

@mcmire mcmire Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just so I understand, you're saying that if we use static versions there is a risk that a dependency on an internal package could point to a published version instead of the internal version, but if we use an unpublished version then we never have that risk?

If so, that's a good point. One immediate thought I have is that this shouldn't be an issue, because you need to use Yarn whenever installing dependencies in this repo, and Yarn seems to be smart about understanding that if package A depends on package B and package B is a workspace package, and the version range of the dependency matches the version of package B in its manifest, it won't attempt to download the published version of package B. This can be observed via the lockfile: https://github.com/MetaMask/core/blob/make-yarn-link-work-again/yarn.lock#L1330

Another thing to think about is that if we added a suffix, we'd have to make sure to strip it when we release. This isn't hard to do in the context of this repo — we can do it in a prepack step — but we'd also need to do it in our GitHub Actions and in create-release-branch, where there are various places that we read the current version of a package by looking directly at its package.json. This would add some complexity to these pieces that could be tricky to maintain, so I'd want to make sure we'd need them.

Copy link
Contributor

@legobeat legobeat Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just so I understand, you're saying that if we use static versions there is a risk that a dependency on an internal package could point to a published version instead of the internal version, but if we use an unpublished version then we never have that risk?

Yup. And there's some history for this happening. Especially since this is a monorepo which includes external dependencies in the same namespace as the monorepo itself (@metamask/), it's an easy enough mistake to make in larger changes, even for those who actually do read scan through each lockfile change manually. I've seen it happen multiple times in other monorepos using this scheme and I suspect it's one reason behind the workspace:^ scheme in the first place.

And yes, it would require some updates to release/publish scripts. But there is already an implicit requirement to bump version precisely once per successful publish.

What is the benefit of having checked-in versions always match the last published version, as opposed to unpublished versions? It seems that you never actually want them to match, and they will start to drift at the first commit after each release despite version numbers matching. It seems to me that this will mean that version-bumping should also change to be done at the time of commit of changes (e.g. bump package to a major at the point of merging a breaking change) as opposed to like now, at point of release.

I may be missing the reasoning behind why these versions specifically are desired, though.

Copy link
Contributor

@legobeat legobeat Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maye @haltman-at / @eggplantzzz have a useful perspective to share given that Truffle seems to already be using the same scheme proposed here.

Copy link
Member

Choose a reason for hiding this comment

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

The constraints mitigate this risk pretty well I'd think. They automatically keep the ranges synchronized with the current versions of each package. Contributors should never have to remember to update them at all.

Copy link
Contributor Author

@mcmire mcmire Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What is the benefit of having checked-in versions always match the last published version, as opposed to unpublished versions?

We developed the release process for the monorepo this way not so much because there was a clear benefit to doing so, but because it evolved from the standard release process that we (and the JavaScript community at large) follow for polyrepos.

What is the benefit of having checked-in versions always match the last published version, as opposed to unpublished versions? It seems that you never actually want them to match, and they will start to drift at the first commit after each release despite version numbers matching. It seems to me that this will mean that version-bumping should also change to be done at the time of commit of changes (e.g. bump package to a major at the point of merging a breaking change) as opposed to like now, at point of release.

I hear what you're saying. I think this might just be a different perspective, however. The checked-in version of a package doesn't mean "the last published version"; rather, it means "the next version to publish". On top of this, our release automation assumes that a package should be published if its version has changed, and that it shouldn't otherwise. This is the reason why we don't bump the version of a package until we want to create a new release. (We are talking about changing this so that contributors can queue version bumps along with changes that necessitate them, but even in that case, I don't think we would actually bump the version of the package in the manifest until release time.)

[...] there's some history for this happening. Especially since this is a monorepo which includes external dependencies in the same namespace as the monorepo itself (@metamask/), it's an easy enough mistake to make in larger changes, even for those who actually do read scan through each lockfile change manually. I've seen it happen multiple times in other monorepos using this scheme and I suspect it's one reason behind the workspace:^ scheme in the first place.

Returning to your first paragraph, I don't want to dismiss this and I definitely appreciate your perspective. That said, do you think that Yarn constraints would adequately protect against what you've seen?

Copy link
Contributor

@legobeat legobeat Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Appreciate the explanation. I did miss the yarn constraints initially, which I agree should address the much of the potential practicalities, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merging since we've got 2 'yes' votes on this and we want to unblock another team we're closely working with, but I don't want to stop the conversation here.

},
"devDependencies": {
"@metamask/auto-changelog": "^3.1.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/approval-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/utils": "^6.2.0",
"eth-rpc-errors": "^4.0.2",
"immer": "^9.0.6",
Expand Down
16 changes: 8 additions & 8 deletions packages/assets-controllers/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@
"@ethersproject/contracts": "^5.7.0",
"@ethersproject/providers": "^5.7.0",
"@metamask/abi-utils": "^2.0.1",
"@metamask/approval-controller": "workspace:^",
"@metamask/base-controller": "workspace:^",
"@metamask/approval-controller": "^3.5.0",
"@metamask/base-controller": "^3.2.0",
"@metamask/contract-metadata": "^2.3.1",
"@metamask/controller-utils": "workspace:^",
"@metamask/controller-utils": "^4.3.1",
"@metamask/eth-query": "^3.0.1",
"@metamask/metamask-eth-abis": "3.0.0",
"@metamask/network-controller": "workspace:^",
"@metamask/preferences-controller": "workspace:^",
"@metamask/network-controller": "^12.1.1",
"@metamask/preferences-controller": "^4.3.0",
"@metamask/rpc-errors": "^5.1.1",
"@metamask/utils": "^6.2.0",
"@types/uuid": "^8.3.0",
Expand Down Expand Up @@ -69,9 +69,9 @@
"typescript": "~4.6.3"
},
"peerDependencies": {
"@metamask/approval-controller": "workspace:^",
"@metamask/network-controller": "workspace:^",
"@metamask/preferences-controller": "workspace:^"
"@metamask/approval-controller": "^3.5.0",
"@metamask/network-controller": "^12.1.1",
"@metamask/preferences-controller": "^4.3.0"
},
"engines": {
"node": ">=16.0.0"
Expand Down
2 changes: 1 addition & 1 deletion packages/composable-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^"
"@metamask/base-controller": "^3.2.0"
},
"devDependencies": {
"@metamask/auto-changelog": "^3.1.0",
Expand Down
8 changes: 4 additions & 4 deletions packages/ens-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@
},
"dependencies": {
"@ethersproject/providers": "^5.7.0",
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/network-controller": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/network-controller": "^12.1.1",
"@metamask/utils": "^6.2.0",
"ethereum-ens-network-map": "^1.0.2",
"punycode": "^2.1.1"
Expand All @@ -48,7 +48,7 @@
"typescript": "~4.6.3"
},
"peerDependencies": {
"@metamask/network-controller": "workspace:^"
"@metamask/network-controller": "^12.1.1"
},
"engines": {
"node": ">=16.0.0"
Expand Down
8 changes: 4 additions & 4 deletions packages/gas-fee-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/eth-query": "^3.0.1",
"@metamask/network-controller": "workspace:^",
"@metamask/network-controller": "^12.1.1",
"@metamask/utils": "^6.2.0",
"@types/uuid": "^8.3.0",
"ethereumjs-util": "^7.0.10",
Expand All @@ -55,7 +55,7 @@
"typescript": "~4.6.3"
},
"peerDependencies": {
"@metamask/network-controller": "workspace:^"
"@metamask/network-controller": "^12.1.1"
},
"engines": {
"node": ">=16.0.0"
Expand Down
8 changes: 4 additions & 4 deletions packages/keyring-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@
},
"dependencies": {
"@keystonehq/metamask-airgapped-keyring": "^0.13.1",
"@metamask/base-controller": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/eth-keyring-controller": "^13.0.0",
"@metamask/message-manager": "workspace:^",
"@metamask/preferences-controller": "workspace:^",
"@metamask/message-manager": "^7.3.0",
"@metamask/preferences-controller": "^4.3.0",
"@metamask/utils": "^6.2.0",
"async-mutex": "^0.2.6",
"ethereumjs-util": "^7.0.10",
Expand All @@ -58,7 +58,7 @@
"uuid": "^8.3.2"
},
"peerDependencies": {
"@metamask/preferences-controller": "workspace:^"
"@metamask/preferences-controller": "^4.3.0"
},
"engines": {
"node": ">=16.0.0"
Expand Down
4 changes: 2 additions & 2 deletions packages/logging-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"uuid": "^8.3.2"
},
"devDependencies": {
Expand Down
4 changes: 2 additions & 2 deletions packages/message-manager/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/eth-sig-util": "^6.0.0",
"@metamask/utils": "^6.2.0",
"@types/uuid": "^8.3.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/network-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/eth-json-rpc-infura": "^8.1.1",
"@metamask/eth-json-rpc-middleware": "^11.0.0",
"@metamask/eth-json-rpc-provider": "^1.0.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/notification-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/utils": "^6.2.0",
"immer": "^9.0.6",
"nanoid": "^3.1.31"
Expand Down
8 changes: 4 additions & 4 deletions packages/permission-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/approval-controller": "workspace:^",
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/approval-controller": "^3.5.0",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/utils": "^6.2.0",
"@types/deep-freeze-strict": "^1.1.0",
"deep-freeze-strict": "^1.1.1",
Expand All @@ -51,7 +51,7 @@
"typescript": "~4.6.3"
},
"peerDependencies": {
"@metamask/approval-controller": "workspace:^"
"@metamask/approval-controller": "^3.5.0"
},
"engines": {
"node": ">=16.0.0"
Expand Down
4 changes: 2 additions & 2 deletions packages/phishing-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@types/punycode": "^2.1.0",
"eth-phishing-detect": "^1.2.0",
"punycode": "^2.1.1"
Expand Down
4 changes: 2 additions & 2 deletions packages/preferences-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^"
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1"
},
"devDependencies": {
"@metamask/auto-changelog": "^3.1.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/rate-limit-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/base-controller": "^3.2.0",
"eth-rpc-errors": "^4.0.2",
"immer": "^9.0.6"
},
Expand Down
10 changes: 5 additions & 5 deletions packages/signature-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@
"test:watch": "jest --watch"
},
"dependencies": {
"@metamask/approval-controller": "workspace:^",
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/message-manager": "workspace:^",
"@metamask/approval-controller": "^3.5.0",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/message-manager": "^7.3.0",
"@metamask/utils": "^6.2.0",
"eth-rpc-errors": "^4.0.2",
"ethereumjs-util": "^7.0.10",
Expand All @@ -50,7 +50,7 @@
"typescript": "~4.6.3"
},
"peerDependencies": {
"@metamask/approval-controller": "workspace:^"
"@metamask/approval-controller": "^3.5.0"
},
"engines": {
"node": ">=16.0.0"
Expand Down
12 changes: 6 additions & 6 deletions packages/transaction-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@
"dependencies": {
"@ethereumjs/common": "^3.2.0",
"@ethereumjs/tx": "^4.2.0",
"@metamask/approval-controller": "workspace:^",
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/approval-controller": "^3.5.0",
"@metamask/base-controller": "^3.2.0",
"@metamask/controller-utils": "^4.3.1",
"@metamask/eth-query": "^3.0.1",
"@metamask/network-controller": "workspace:^",
"@metamask/network-controller": "^12.1.1",
"@metamask/utils": "^6.2.0",
"async-mutex": "^0.2.6",
"eth-method-registry": "1.1.0",
Expand All @@ -59,8 +59,8 @@
"typescript": "~4.6.3"
},
"peerDependencies": {
"@metamask/approval-controller": "workspace:^",
"@metamask/network-controller": "workspace:^",
"@metamask/approval-controller": "^3.5.0",
"@metamask/network-controller": "^12.1.1",
"babel-runtime": "^6.26.0"
},
"engines": {
Expand Down
Loading