-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat: show warning for dist tags #38
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new diagnostic rule that warns developers when dependencies use dist-tags (such as latest, next, or beta) rather than pinned versions. The implementation includes a new configuration option Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
|
||
| const tag = parsed.semver | ||
| const isPublishedDistTag = tag in (pkg.distTags ?? {}) | ||
| const isCommonDistTag = COMMON_DIST_TAGS.has(tag.toLowerCase()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we check for COMMON_DIST_TAGS? I might be missing some specific use cases here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pretty much every project I've worked on before, using a dist tag for a version was considered bad practice because it can lead to all sorts of problems (stale installations, keeping track of changes in deps, package updates breaking apps, etc.). It would be nice to get a warning about this, because a lot of boilerplate/starter apps use dist tags in their package.json (for example when setting up a project with pnpm create).
This might not be the general expectation or experience shared by other developers, and I'm curious what your opinion on this is. I think having the option for a warning would be nice, and I'm fine with it being off by default :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree that using dist‑tags for version pinning is not a good practice.
Just curious why do we check for isCommonDistTag rather than warning on any dist‑tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh gotcha that went completely over my head. My thought was that if the dist tag is missing in from the package metadata we have a fallback based on the most common ones. But now that I think about it, a check if its just an alphanumeric string would actually solve this and not let through any uncommon dist tags...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/package.ts (1)
17-31:⚠️ Potential issue | 🟡 MinorFix false positives for v-prefixed semver versions in dist-tag detection.
The current pattern treats any alpha-leading token as a dist tag. Since
parseVersion()does not strip the leadingv, a pinned version likev1.2.3would match and trigger an incorrect warning (npm's semver parser accepts and strips thevprefix, but this code does not). Users would see a misleading message claiming a specific version is a distribution tag.Adjust the pattern to exclude v-prefixed semver strings:
Suggested fix
-const DIST_TAG_PATTERN = /^[a-z][\w.-]*$/i +// Exclude v-prefixed semver to avoid false positives (e.g., "v1.2.3"). +const DIST_TAG_PATTERN = /^(?!v?\d+\.\d+\.\d+(?:[-+][\w.-]+)?$)[a-z][\w.-]*$/i
|
Thanks! Can you please resolve the conflicts and check if #38 (review) is valid? |
Closes #37