Use static versions for interdependencies

[mcmire]

Explanation

If you have a project that depends on a package in this monorepo and you want to test changes to the package locally, you cannot simply configure that project to use a local reference to that package. Instead, you must use a preview build of the package. While publishing that preview build is a relatively easy process, getting the project to use that preview build is not.

The reason why preview builds are needed is because of the way that interdependencies work. If a package in the monorepo needs to depend on another package in the monorepo, it uses workspace:^ as the version. Yarn, which understands the workspace: protocol, will resolve this reference to the version of whatever the dependency package happens to be when the dependent package is published.

But if no packages are published, such as the case when testing locally, then workspace: references will never be resolved. If an attempt is used to update the dependency on the package in question — let's say @metamask/base-controller — by replacing the version with a file: or portal: reference, Yarn will produce an error that looks like:

➤ YN0001: │ Error: @metamask/base-controller@workspace:^: Workspace not found (@metamask/base-controller@workspace:^)

To solve this problem, this commit replaces the workspace: references with static references and adds a Yarn constraint to ensure that all interdependencies match their actual versions. That is, if transaction-controller depends on base-controller, then the version it uses is guaranteed to match the version in base-controller's manifest.

References

Fixes #1586.

Changelog

(No consumer-facing changes)

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

[socket-security]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/preferences-controller	4.3.0	None	+4	506 kB	metamaskbot
@metamask/approval-controller	3.5.0	None	+4	595 kB	metamaskbot
@metamask/controller-utils	4.3.1	None	+0	112 kB	metamaskbot
@metamask/base-controller	3.2.0	None	+0	122 kB	metamaskbot
@metamask/message-manager	7.3.0	None	+4	620 kB	metamaskbot
@metamask/network-controller	12.1.1	environment	+8	1.14 MB	metamaskbot

[legobeat]

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.

[mcmire]

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.

[legobeat]

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.

[legobeat]

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

[Gudahtt]

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.

[mcmire]

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?

[legobeat]

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

[mcmire]

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.

[Gudahtt]

Looks great! We should have some documentation about how this workflow works though

[mcmire]

@Gudahtt Okay, I've updated the docs. Let me know if anything doesn't make sense!

[Gudahtt]

LGTM!