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

Ensure workspace: prefix is updated correctly when using PNPM. #110

Conversation

NullVoxPopuli
Copy link
Contributor

I was trying to publish glimmer-vm, and ran in to an issue, noted here: #79 (comment)

I believe this is a reasonable way to move forward with pnpm + "workspace protocol" support, as its opt-in, and existing behavior is unchanged.

lemme know what you think, thanks!

@NullVoxPopuli NullVoxPopuli changed the title Support the workspace protocol (as well as other things) Support the workspace protocol (or other workflows that update references out of band) Oct 13, 2023
@rwjblue
Copy link
Collaborator

rwjblue commented Oct 16, 2023

Hmmm. Seems like we should make using workspace: "just work" (IOW, we shouldn't require an opt-in).

Can you create a test here that emulates the repo structure?

@NullVoxPopuli
Copy link
Contributor Author

, we shouldn't require an opt-in

Fwiw, the way glimmer-vm is setup now is that release it isn't used at all for releasing, but we use it for everything else it does. We have a gh workflow that'll release when a tag occurs

@rwjblue
Copy link
Collaborator

rwjblue commented Oct 16, 2023

I think we probably need two things:

  1. allow folks to configure what command to actually use for publishing (instead of always using npm publish here) -- pnpm publish is required to support these workspace: prefixes
  2. when we figure out what version to use here we need it to be aware of workspace: and preserve it properly (but it should still update the version part)

@NullVoxPopuli
Copy link
Contributor Author

  1. seesm like separate work -- I can open a PR for this tho -- a config for switching npm publish to pnpm publish doesn't address my use case
  2. I can remove the config option, and just detect "workspace:" and not touch it. I'll do that in this PR

@NullVoxPopuli
Copy link
Contributor Author

PR for #1 here: #111

@NullVoxPopuli
Copy link
Contributor Author

#2 is done in this PR, thanks for reviewing!

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@IvanLi-CN
Copy link

Any progress?
I have used this PR in the pnpm workspace project and feel good so far ❤️

index.js Outdated
// tools that use this replace with a real version during the publish the process
if (existingVersion.startsWith('workspace:')) {
prefix = 'workspace:';
range = existingVersion.split(':')[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very very slightly off. Since we are only checking that it starts with workspace:, we need to slice the beginning instead of split on : (otherwise, any values that include multiple : would be mangled incorrectly -- I realize this may not actually be a real thing, but there is no reason to diverge between our guard and the final value).

Updated to existingVersion.slice(prefix.length) instead.

@rwjblue rwjblue changed the title Support the workspace protocol (or other workflows that update references out of band) Ensure workspace: prefix is updated correctly when using PNPM. Jan 15, 2024
@rwjblue rwjblue added the bug Something isn't working label Jan 15, 2024
@rwjblue rwjblue merged commit 7c24eeb into release-it-plugins:master Jan 15, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants