Skip to content

ci(after-merge): fix NPM publishing#11599

Closed
turadg wants to merge 1 commit intomasterfrom
11598-publish-dev
Closed

ci(after-merge): fix NPM publishing#11599
turadg wants to merge 1 commit intomasterfrom
11598-publish-dev

Conversation

@turadg
Copy link
Member

@turadg turadg commented Jul 14, 2025

refs: #11598

Description

NPM packages since this PR have had workspace:* in their package.json (example):

Those can't be installed. This uses Yarn to resolve the versions per #11598 (comment)

Security Considerations

We might want to enable --provenance (docs), which Yarn supports while Lerna didn't, but I think that's out of scope.

Scaling Considerations

n/a

Documentation Considerations

aligning with ecosystem, less to document. the new lines reference the documentation.

Testing Considerations

Up to the reviewers whether to merge this and see if it works or manually run the workflow.

This shouldn't affect the SDK release process, which is why it doesn't close #11598 , but there may be other pre-release cases to consider.

Upgrade Considerations

n/a

@turadg turadg requested review from mhofman and mujahidkay July 14, 2025 21:21
@turadg turadg requested a review from a team as a code owner July 14, 2025 21:21
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

This seems to be missing the crucial step of actually bumping the version before publishing.

git stash pop || true
# use Yarn to resolve `workspace:*` versions and its "npm" plugin to publish
# https://yarnpkg.com/features/workspaces#publishing-workspaces
yarn workspaces foreach --all --topological --no-private npm publish --tolerate-republish --tag $TAG
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand, how is the version bumped here?

tolerate-republish will simply avoid erroring when the version already exists in a registry, but it will not actually publish.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't bump the version. That's why I said it wouldn't work for releases. It just fixes the after-merge packages which I believe include the git sha in their versions.

Separately we can improve the version bumping process.

Copy link
Member

Choose a reason for hiding this comment

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

I believe include the git sha in their versions

It only includes the git sha in their version because of the lerna command being removed here. This would only attempt to publish the package with the currently checked in version of each package, which can only work once.

@mhofman
Copy link
Member

mhofman commented Jul 15, 2025

Reading more about yarn workspace and publishing, is there a reason we went for workspace:* and not for workspace:^ ? Worried that * resolution implies a strict = version, potentially leading to more agoric-sdk package duplication in downstream consumer.

@turadg
Copy link
Member Author

turadg commented Jul 15, 2025

Reading more about yarn workspace and publishing, is there a reason we went for workspace:* and not for workspace:^ ?

I had figured:

  1. Agoric SDK is not disciplined in its use of semver
  2. Projects almost always want a coherent set of package versions from one SDK state
  3. When they don't they can use resolutions

On balance I think the risk is worse to think you're getting compatible versions and then not because of a semver error. But if you want to change the range specifier I wouldn't request changes.

@turadg
Copy link
Member Author

turadg commented Jul 15, 2025

To be clear, I'm still on leave and this PR was a proposal but whoever is closest to this situation should take it over. I won't be merging it. Close at will.

@mhofman
Copy link
Member

mhofman commented Jul 15, 2025

On balance I think the risk is worse to think you're getting compatible versions and then not because of a semver error.

Yeah the poor adherence of agoric-sdk with semver is concerning, but the headache of dealing with duplicate deps shouldn't IMO be dismissed. We've ourselves encountered issues when endo or cosmjs were using strict semver for their packages.

Anyway, since we're still using pre-release semver versions, I think this point is moot for now, and we can postpone this to when we move away from pre-release semver and conventional commits.

if you want to change the range specifier I wouldn't request changes.

To be clear, the change requested is about publishing without version bumps, which as proposed simply wouldn't work. The concern about range specifier came after, and is not blocking.

this PR was a proposal but whoever is closest to this situation should take it over.

I think the best bet is roll the idea of using yarn npm publish into the lerna 8 attempt, or move away from conventional commits altogether. Thanks for raising this possibility.

@mhofman
Copy link
Member

mhofman commented Jul 24, 2025

afaiu, doing yarn npm publish is not necessary with lerna 8, and this PR is no longer needed, right?

@turadg turadg closed this Jul 24, 2025
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.

NPM packages broken since adopting workspace protocol

2 participants