Skip to content

Conversation

@nitodeco
Copy link
Collaborator

@nitodeco nitodeco commented Feb 9, 2026

Closes #37

lenarlaiscore Consider pinning to a specific version, apadistotan Nomx › Diagnostics Dist Tac

@nitodeco nitodeco requested a review from 9romise February 9, 2026 22:44
@coderabbitai
Copy link

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This 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 npmx.diagnostics.distTag (defaulting to true), a checkDistTag rule that detects and reports dist-tag usage, a helper function isDistTagLike for version identification, and comprehensive test coverage. The rule returns diagnostic warnings with guidance to pin to specific versions when dist-tags are detected in dependencies.

Possibly related PRs

  • PR #30: Both touch version parsing and protocol support in src/utils/package.ts, with the new dist-tag diagnostic depending on these utilities for parsing and validation.
  • PR #18: The new dist-tag diagnostic integrates with the existing diagnostics pipeline (src/providers/diagnostics/index.ts) and interacts with DiagnosticRule and PackageInfo types modified in that PR.
  • PR #29: The new dist-tag diagnostic depends on version parsing and formatting utilities (parseVersion/formatVersion) that were added or changed in that PR.

Suggested reviewers

  • 9romise
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The PR description adequately references the linked issue (#37) and describes the feature implementation for showing warnings on dist-tag dependencies.
Linked Issues check ✅ Passed The PR successfully implements all requirements from issue #37: detecting dist-tag usage and showing warnings to encourage pinning to specific versions.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the dist-tag warning feature; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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 @coderabbitai help to get the list of available commands and usage tips.


const tag = parsed.semver
const isPublishedDistTag = tag in (pkg.distTags ?? {})
const isCommonDistTag = COMMON_DIST_TAGS.has(tag.toLowerCase())
Copy link
Member

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.

Copy link
Collaborator Author

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 :)

Copy link
Member

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?

Copy link
Collaborator Author

@nitodeco nitodeco Feb 12, 2026

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...

Copy link

@coderabbitai coderabbitai bot left a 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 | 🟡 Minor

Fix 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 leading v, a pinned version like v1.2.3 would match and trigger an incorrect warning (npm's semver parser accepts and strips the v prefix, 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

@9romise
Copy link
Member

9romise commented Feb 12, 2026

Thanks! Can you please resolve the conflicts and check if #38 (review) is valid?

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.

Show warning when using dist-tags

2 participants