fix(intent): read local package.json version before npm registry fallback#104
fix(intent): read local package.json version before npm registry fallback#104RazorAlexMachin wants to merge 1 commit intoTanStack:mainfrom
Conversation
…back Fixes TanStack#103 — fetchNpmVersion hardcodes registry.npmjs.org, which returns 404 for private/scoped packages (GitHub Packages, Artifactory, etc.), breaking version drift detection. Changes: - Add readLocalVersion() to read package.json from packageDir - Add fetchCurrentVersion() that tries local first, then npm registry - Update checkStaleness() to use the new fallback chain - Add test: private package with npm 404 → reads local version - Add test: local package.json takes precedence over npm registry
📝 WalkthroughWalkthroughThe pull request implements a two-step version resolution strategy in the staleness checker. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/intent/src/staleness.ts (1)
45-45: Guard against empty localversionvalues before suppressing fallback.A blank string currently counts as a valid local version, which can skip npm fallback and produce unreliable drift classification.
Proposed small hardening
- return typeof pkgJson.version === 'string' ? pkgJson.version : null + return typeof pkgJson.version === 'string' && pkgJson.version.trim() !== '' + ? pkgJson.version + : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/staleness.ts` at line 45, The current return treats an empty string as a valid local version (returning pkgJson.version) which can suppress the npm fallback; update the check where pkgJson.version is returned to ensure it is a non-empty string (e.g., typeof pkgJson.version === 'string' && pkgJson.version.trim().length > 0) and return null if the version is empty or whitespace so downstream drift classification will fall back to npm; target the return handling of pkgJson.version in packages/intent/src/staleness.ts (the place that currently does "return typeof pkgJson.version === 'string' ? pkgJson.version : null").packages/intent/tests/staleness.test.ts (1)
325-333: Assert fetch is not called when localpackage.jsonversion exists.This tightens the contract from “local wins” to “local short-circuits network fallback.”
Proposed assertion additions
const report = await checkStaleness(tmpDir, '@private/lib') expect(report.currentVersion).toBe('2.5.0') expect(report.versionDrift).toBe('minor') + expect(globalThis.fetch).not.toHaveBeenCalled() const skill = requireFirstSkill(report) expect(skill.needsReview).toBe(true) expect(skill.reasons[0]).toContain('version drift') @@ const report = await checkStaleness(tmpDir, '@example/lib') // Local package.json should take precedence expect(report.currentVersion).toBe('3.0.0') expect(report.versionDrift).toBe('major') + expect(globalThis.fetch).not.toHaveBeenCalled()Also applies to: 347-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/staleness.test.ts` around lines 325 - 333, The test should assert the network fetch is never invoked when a local package.json version exists: after calling checkStaleness(tmpDir, '@private/lib') (the test using mockFetchNotOk and requireFirstSkill), add an assertion that the mocked fetch function was not called (e.g., expect(fetchMock).not.toHaveBeenCalled() or the project's equivalent mock used by mockFetchNotOk). Do the same addition in the other similar test block (around the other checkStaleness case at lines 347-354) to ensure the local short-circuits network fallback behavior is enforced; reference the mock helper mockFetchNotOk and the function under test checkStaleness to locate where to place the new assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/intent/src/staleness.ts`:
- Line 45: The current return treats an empty string as a valid local version
(returning pkgJson.version) which can suppress the npm fallback; update the
check where pkgJson.version is returned to ensure it is a non-empty string
(e.g., typeof pkgJson.version === 'string' && pkgJson.version.trim().length > 0)
and return null if the version is empty or whitespace so downstream drift
classification will fall back to npm; target the return handling of
pkgJson.version in packages/intent/src/staleness.ts (the place that currently
does "return typeof pkgJson.version === 'string' ? pkgJson.version : null").
In `@packages/intent/tests/staleness.test.ts`:
- Around line 325-333: The test should assert the network fetch is never invoked
when a local package.json version exists: after calling checkStaleness(tmpDir,
'@private/lib') (the test using mockFetchNotOk and requireFirstSkill), add an
assertion that the mocked fetch function was not called (e.g.,
expect(fetchMock).not.toHaveBeenCalled() or the project's equivalent mock used
by mockFetchNotOk). Do the same addition in the other similar test block (around
the other checkStaleness case at lines 347-354) to ensure the local
short-circuits network fallback behavior is enforced; reference the mock helper
mockFetchNotOk and the function under test checkStaleness to locate where to
place the new assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8087092-67f4-4868-9ba3-f331c2d69675
📒 Files selected for processing (3)
.changeset/fix-local-version-fallback.mdpackages/intent/src/staleness.tspackages/intent/tests/staleness.test.ts
|
View your CI Pipeline Execution ↗ for commit 6b4b8f5
☁️ Nx Cloud last updated this comment at |
commit: |
🎯 Changes
Read the local
package.jsonversion incheckStalenessbefore falling back to the npm registry fetch. This fixes version drift detection for packages not published to publicregistry.npmjs.org(e.g. GitHub Packages, Artifactory, private registries).Fixes #103
Problem
fetchNpmVersion()hardcodeshttps://registry.npmjs.org/— any scoped/private package not on the public registry returns a 404, causingcurrentVersionto always benulland silently disabling version drift detection.Solution
readLocalVersion(packageDir)— readsversionfrom the localpackage.jsonfetchCurrentVersion(packageDir, packageName)— tries local first, falls back to npm registrycheckStaleness()to use the new fallback chainThis works for both use cases:
package.jsondirectly (works for all registry types)node_modules: readspackage.jsonfrom the installed package directoryTests
Two new test cases added to
staleness.test.ts:reads version from local package.json when npm fetch fails— simulates private package (npm 404)prefers local package.json over npm registry— verifies local takes precedenceAll 17 tests pass.
✅ Checklist
pnpm vitest run packages/intent/tests/staleness.test.ts.🚀 Release Impact
Summary by CodeRabbit