-
-
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 distribution tags (such as latest, next, and beta) instead of pinned versions. The changes include a new configuration option npmx.diagnostics.distTag, a new diagnostic rule implementation in src/providers/diagnostics/rules/dist-tag.ts, a constant set of common distribution tags, integration of the rule into the diagnostics pipeline, comprehensive test coverage, and updates to the Vitest configuration to use tsconfig-paths for path resolution. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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. 🎉 🧹 Recent nitpick comments
Comment |
| @@ -1,18 +1,18 @@ | |||
| import { join } from 'node:path' | |||
| import { fileURLToPath } from 'node:url' | |||
| import tsconfigPaths from 'vite-tsconfig-paths' | |||
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.
We could put the vite-tsconfig-paths change in its own PR.
|
|
||
| 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?
Closes #37