Skip to content

fix: resolve @webjsdev versions through Node so a hoisted install passes - #1351

Merged
vivek7405 merged 3 commits into
mainfrom
fix/doctor-versions-hoist
Aug 8, 2026
Merged

fix: resolve @webjsdev versions through Node so a hoisted install passes#1351
vivek7405 merged 3 commits into
mainfrom
fix/doctor-versions-hoist

Conversation

@vivek7405

@vivek7405 vivek7405 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Part 3 of #1300, the priority slice. This is PR 1 of three sequenced against that issue, so it deliberately carries no Closes line and #1300 stays open for parts 1 and 2.

webjs doctor's WEBJS_VERSIONS check read <appDir>/node_modules/<dep>/package.json to find a declared dependency's installed version. Under npm workspaces the @webjsdev/* deps hoist to the ROOT node_modules, so an app subdirectory has no local copy and every declared dep came back reported as not installed on a perfectly healthy install. Both in-repo apps warned that way (examples/blog named all four deps, website all three), while FRAMEWORK_RESOLVE in the same output said @webjsdev/core resolves from the app directory, so the two checks openly contradicted each other. It also made the check ungatable: no app could set "WEBJS_VERSIONS": "error" without redding CI on a healthy tree.

It now asks Node's resolver, anchored at the app dir. That is the question the check actually asks ("would this app resolve this dependency at runtime, and at what version"), so it needs no hoist-awareness of its own and picks up symlinked workspace links, nested non-hoisted trees, and imports for free.

What changed

  • readInstalledVersion(dep, appDir), a module-private helper in packages/cli/lib/doctor.js, resolving through createRequire anchored at the app dir. Both halves of the resolve are load-bearing and neither works alone: @webjsdev/cli is bin-only, so require.resolve('@webjsdev/cli') throws MODULE_NOT_FOUND and only the direct <dep>/package.json attempt finds it, while @webjsdev/server locks ./package.json out of its exports map, so the direct attempt is refused with ERR_PACKAGE_PATH_NOT_EXPORTED and only the main-entry plus bounded-walk fallback finds it.
  • The loop body in checkWebjsVersions calls it. Every message string, the pass / warn statuses, the bestEffort posture, DOCTOR_CODES, and the satisfiesRange null (unverifiable range shape) treatment are untouched. The check's contract does not change, only its resolution.
  • Six fixtures in test/cli/doctor.test.mjs, plus a strengthened assertion on the existing missing-dep case so it names the dep.
  • packages/cli/AGENTS.md, the webjs doctor row, recording how the version is resolved and why each half of the fallback exists.

The helper is local rather than getPackageVersion from @webjsdev/server for two reasons: doctor must stay usable when the framework does not resolve from the app dir at all, which is the #954 fresh-worktree case doctor exists to diagnose, and that helper resolves the main entry only, so it returns null for a bin-only package and would leave @webjsdev/cli reported missing anyway.

No webjs.doctor.gate block and no default severity moved. The deliverable is that the check becomes gatable; whether to gate it is the app's call, and gating it now would be a no-op today and an untested trap later.

Test plan

  • test/cli/doctor.test.mjs: 106 pass. New fixtures cover the workspace-hoisted app (pass, counting every dep), a bin-only package, an exports-locked package, drift still reported through a hoisted resolve (an undefined version could not produce a drift message, so that case doubles as the non-undefined proof), a workspace app declaring a dep nothing installed (still warn, naming it), and an unverifiable range shape (still no warn).
  • Counterfactual, proven at 329572c1 and re-proven at 21812bd8: restore the join(appDir, 'node_modules', dep, 'package.json') read and all six new fixtures red with "N @webjsdev/* dependency not installed", which is the measured before-state on both apps.
  • webjs doctor --json from examples/blog reports WEBJS_VERSIONS pass, All 4 @webjsdev/* dependency satisfy their declared ranges, and from website the same with 3. Before the fix, the same command in the primary checkout reports warn, 4 @webjsdev/* dependency not installed: @webjsdev/cli, @webjsdev/core, @webjsdev/server, @webjsdev/intellisense.
  • webjs check passes and webjs doctor exits 0 in both apps.
  • npm test: 4119 of 4125 pass. The five failures are environmental to a linked worktree and unrelated to this diff. packages/server/test/elision/differential-elision.test.js fails the same way in an unrelated worktree at a different base commit and passes in the primary checkout (the fixture app renders 500 there), and the two test/bun/* wrappers fail in the primary checkout on main too. CI builds from the branch with a real install, so it is the authority on those.
  • A second counterfactual pins the resolve ORDER rather than the resolution: force the entry-first path and the bin-only case reds. That case only discriminates because the fixture builder writes an entry file solely for a manifest declaring main or exports, so a bin-only manifest has nothing to fall back to, which is what a real bin-only package looks like.
  • Dogfood: website boots in prod mode through createRequestHandler and serves 200 on /, /docs/configuration, /ui, and /ui/button, with 7, 12, 12, and 50 modulepreload hints and none broken. examples/blog is covered by the e2e suite, which this diff does not touch.
  • Browser and e2e: N/A. This is Node-side CLI tooling with no browser-facing surface.
  • Bun parity: N/A. AGENTS.md's runtime-sensitive list is the serializer, the listener and request path, SSR / action / CSRF dispatch, streams, node:crypto, the TS stripper, and auth / session / cors. A createRequire probe in a CLI check is on none of them, and the doc states the check tooling stays on Node.
  • Docs surfaces: packages/cli/AGENTS.md updated. Root AGENTS.md:525 does not enumerate WEBJS_VERSIONS, and website/app/docs/configuration/page.ts:62 describes it only as "@webjsdev/* version coherence", both of which stay accurate, so neither changed. No scaffold, MCP, editor-plugin, or README surface touches this check.

webjs doctor's WEBJS_VERSIONS check read
<appDir>/node_modules/<dep>/package.json directly. Under npm workspaces the
@webjsdev/* deps hoist to the root node_modules, so an app subdirectory has no
local copy and every declared dep was reported not installed on a perfectly
healthy install. Both in-repo apps warned that way, while FRAMEWORK_RESOLVE in
the same output said the framework resolves fine, so the two checks openly
contradicted each other.

Ask Node's resolver instead, anchored at the app dir. That is the question the
check actually asks, so it needs no hoist-awareness of its own and picks up
symlinked workspace links and nested trees for free. Both halves of the resolve
are load-bearing: @webjsdev/cli is bin-only so it has no main entry, and
@webjsdev/server locks ./package.json out of its exports map.

The check now becomes gatable, which it could not be while it warned on a
healthy install.
The workspace fixture builder wrote an index.js for every package, so the
bin-only case had no main and no exports but did have an entry file to fall
back to. require.resolve('<dep>') therefore succeeded and the case stopped
pinning the resolve ORDER it exists to pin: swapping the two attempts left it
green. Write an entry file only for a manifest that declares main or exports.

Verified by the swap: entry-first ordering now reds the bin-only case.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the whole diff. The resolution change itself is right: the check was asking a resolution question with a directory read, and asking Node instead is the only version of this that stays correct as install layouts change. I checked the two-half resolve is genuinely load-bearing rather than defensive padding, and it is, on both real packages.

One real problem, in the tests rather than the source. The bin-only fixture was not bin-only, so the case that claims to pin the resolve ORDER did not pin it, and the docblock in doctor.js says it does. That is the kind of test that reads as coverage and is not. Fixed by writing an entry file only where the manifest declares one, and I proved the fixed fixture discriminates by swapping the two attempts and watching it red.

The other note is a miscount in the counterfactual comment. Both are fixed on the branch.

Comment thread test/cli/doctor.test.mjs
Comment thread test/cli/doctor.test.mjs
@vivek7405
vivek7405 marked this pull request as ready for review August 8, 2026 17:28
@vivek7405
vivek7405 merged commit d4226eb into main Aug 8, 2026
10 checks passed
@vivek7405
vivek7405 deleted the fix/doctor-versions-hoist branch August 8, 2026 19:25
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.

1 participant