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

Duplicate Tag + Release created without v Prefix #738

Open
karlhorky opened this issue Mar 15, 2024 · 10 comments
Open

Duplicate Tag + Release created without v Prefix #738

karlhorky opened this issue Mar 15, 2024 · 10 comments

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Mar 15, 2024

Description

Publishing a new version of our package with [email protected] (with pnpm) caused a missing v prefix on the GitHub release name (observe the missing v in the first URL below).

Digging in deeper reveals that two Git tags were created v1.1.0 and 1.1.0, one with GitHub Release with release notes, one without:

stylelint-config-upleveled Tags:

Logs from the publish:

➜  stylelint-config-upleveled git:(main) np --version
10.0.1
➜  stylelint-config-upleveled git:(main) np

Publish a new version of stylelint-config-upleveled (current: 1.0.7)

Commits:
- Ignore @theme for Tailwind CSS v4  f0701c0
- Update dependency typescript to v5.4.2 (#71)  75e287e
- Update dependency eslint to v8.57.0 (#70)  ddeefe5
- Update dependency upgrades - non-major (#69)  4676887
- Update dependency eslint-config-upleveled to v7.8.0 (#68)  bb095ba
- Update dependency eslint-config-upleveled to v7.7.1 (#67)  bec07e3
- Update dependency upgrades - non-major (#66)  cafded1
- Update dependency stylelint to ^16.2.0 (#65)  8d8847b
- Bump vite from 5.0.8 to 5.0.12 (#64)  09c9051
- Update dependency vitest to v1.2.1 (#63)  492160b
- Update dependency vitest to v1.1.3 (#62)  a80a9d1
- Update dependency upgrades - non-major (#61)  0b6fa47

Commit Range:
v1.0.7...main

Registry:
https://registry.npmjs.org/

? Select SemVer increment or specify new version minor 	1.1.0

  ✔ Prerequisite check
  ✔ Git
  ✔ Installing dependencies using pnpm
  ✔ Running tests
  ✔ Bumping version
  ✔ Publishing package
  ✔ Pushing tags
  ✔ Creating release draft on GitHub

 stylelint-config-upleveled 1.1.0 published 🎉

Is this intentional? I could not see this change in the 10.0.0 release notes

Maybe this was something introduced by the pnpm support PR by @mmkal or the followup pnpm commits by @tommy-mitchell?

Previous Behavior

The previous versions of np created tags with the v prefix, such as v1.0.7 here:

Steps to reproduce

  1. Publish new minor version of stylelint-config-upleveled by running np
  2. Accept the default settings in the GitHub Release create form when it loads in the browser
  3. Observe tag created with v prefix (without GitHub Release + release notes)
  4. Observe duplicate tag created without v prefix (and with GitHub Release + release notes)

Expected behavior

Only a single tag + GitHub Release should be created, with a v prefix and containing the release notes

Environment

np - 10.0.1
Node.js - 20.11.1
npm - 10.2.4
pnpm - 8.15.3
Git - 2.39.2
OS - macOS Sonoma 14.4

@karlhorky karlhorky changed the title v Prefix not Added to Git Tag v Prefix not Added to version in GitHub Release name Mar 15, 2024
@karlhorky karlhorky changed the title v Prefix not Added to version in GitHub Release name Duplicate Tag + Release created without v Prefix Mar 15, 2024
@mmkal
Copy link
Collaborator

mmkal commented Mar 15, 2024

Definitely sounds wrong that two separate tags are being created. We use pnpm config get tag-version-prefix for pnpm, which in my case is empty string, whereas npm config get tag-version-prefix is 'v'.

Might be that somewhere is hard-coding a 'v'.

Probably the easiest workaround right now is to set the pnpm config value to 'v', until it's fixed. We probably should keep the no-prefix behaviour for pnpm but definitely shouldn't have two separate tags.

@karlhorky
Copy link
Contributor Author

Interesting, I think I may have found more information - during publishing it opens the browser to this URL, which is creating a new tag without the v prefix (2 occurrences without v here):

https://github.com/upleveled/eslint-config-upleveled/releases/new?tag=7.8.2&body=-+Switch+module+config+in+...%0A%0Ahttps%3A%2F%2Fgithub.com%2Fupleveled%2Feslint-config-upleveled%2Fcompare%2Fv7.8.1...7.8.2&prerelease=false

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 15, 2024

@mmkal maybe line 11 here?

import open from 'open';
import newGithubReleaseUrl from 'new-github-release-url';
import {getTagVersionPrefix, getPreReleasePrefix} from './util.js';
import Version from './version.js';
const releaseTaskHelper = async (options, pkg, pkgManager) => {
const newVersion = options.releaseDraftOnly
? new Version(pkg.version)
: new Version(pkg.version).setFrom(options.version.toString(), {prereleasePrefix: await getPreReleasePrefix(pkgManager)});
const tag = await getTagVersionPrefix(pkgManager) + newVersion.toString();
const url = newGithubReleaseUrl({
repoUrl: options.repoUrl,
tag,
body: options.releaseNotes(tag),
isPrerelease: newVersion.isPrerelease(),
});

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 15, 2024

We probably should keep the no-prefix behaviour for pnpm

Hm... my gut feel would be to add prefixes everywhere.

Seems very surprising that if I use pnpm, I suddenly get an unusual, very uncommon tag format for my releases. (I have seen v prefixes pretty much everywhere on GitHub Releases, can't remember a JS/TS ecosystem package that does not use this prefix)

Even pnpm itself uses v prefixes for their GitHub Releases + tags:

@mmkal
Copy link
Collaborator

mmkal commented Mar 15, 2024

We probably should keep the no-prefix behaviour for pnpm

Hm... my gut feel would be to add prefixes everywhere.

I mean more that we should respect pnpm config get tag-version-prefix - ignoring the config and forcing 'v' would be a breaking change, but I guess one we could make. I'm not sure why pnpm is getting a different result from npm config get, though. Maybe that's a pnpm bug?

@karlhorky
Copy link
Contributor Author

Yeah, good question why it's returning something different. 🤔

I would lean towards doing the change as a patch release (even if it's technically a breaking change) since the behavior feels like a bug and creates unexpected release names on GitHub.

Maybe @sindresorhus has an opinion.

@mmkal
Copy link
Collaborator

mmkal commented Mar 20, 2024

Thinking about it more, I now agree. We should just use npm to get the tag prefix since pnpm doesn't ship with a default v. I'm pretty sure that it's what the majority of users would want, and it's still easy enough to do npm config set tag-version-prefix '' if you don't like it.

It still must be a bug that there's a duplicate, but that's a separate, and smaller, problem.

mmkal added a commit that referenced this issue Mar 20, 2024
@mmkal
Copy link
Collaborator

mmkal commented Mar 20, 2024

Opened a PR for it: #739. I also think that this should be considered a bugfix rather than a real breaking change. I will try it out on another package later this week. @karlhorky you should be able to also try it by adding something like "np": "github:sindresorhus/np#pnpm-vs-npm-tag-version-prefix" to your package.json dependencies.

FYI @sindresorhus

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 27, 2024

Nice, I saw that #739 was reviewed and merged and released in [email protected], thanks @mmkal and @sindresorhus !

I just tested [email protected] with a project, and it's working as it was pre-v10 🎉 (no duplicate tags)

From my side, this issue could be closed, but maybe @mmkal you still have identified another issue which is why we should keep it open?

@mmkal
Copy link
Collaborator

mmkal commented Mar 27, 2024

I suspect we'd have duplicate tags if someone manually set their tag-version-prefix. I haven't tested but let's keep open until someone can.

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

No branches or pull requests

2 participants