fix(path-checker): resolve workspace aliases and filter URLs - #17
fix(path-checker): resolve workspace aliases and filter URLs#17adelin-b wants to merge 1 commit into
Conversation
The path checker was flagging workspace package references (e.g. `@acme/ui`, `@acme/shared/utils`) as MISSING_PATH errors because it only checked the filesystem. In monorepos, these are valid import aliases that resolve via the package manager. Changes: - Add workspace name collection from root package.json `workspaces` field — reads each workspace's package.json `name` to build a lookup set. Works with all package managers (npm, yarn, pnpm, bun). - Try Node's `require.resolve` first for installed npm packages, fall back to workspace name check for package managers that don't symlink all workspaces into node_modules (e.g. bun). - Filter URLs (http://, https://) which are never filesystem paths. Example: a monorepo with `packages/ui/package.json` containing `"name": "@acme/ui"` will now correctly resolve references like `@acme/ui/button` in scaffold files instead of reporting them as missing paths.
theDakshJaitly
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The approach is solid: clean separation with collectWorkspaceNames, good normalization of the workspaces field, and the require.resolve → workspace fallback chain is well-structured.
A few things to address before merging:
HIGH: pnpm workspace support missing
The JSDoc says "Works with any package manager (npm, yarn, pnpm, bun)" but pnpm uses pnpm-workspace.yaml, not the workspaces field in package.json. This means pnpm monorepos will get an empty workspace set and still produce false-positive MISSING_PATH errors.
Fix: Either parse pnpm-workspace.yaml as a fallback when patterns is empty, or update the comment to exclude pnpm.
MEDIUM: URL regex only covers http/https
URL_PATTERN won't catch ftp://, file:///, or protocol-relative // URLs. Consider broadening to:
const URL_PATTERN = /^(?:https?|ftp|file):\/\//;MEDIUM: createRequire anchor path
createRequire(resolve(projectRoot, "package.json")) works incidentally but the canonical pattern is createRequire(resolve(projectRoot, "noop.js")) or using pathToFileURL(), since createRequire expects a module filename.
LOW: No tests for new behavior
There are no tests for the path checker module at all. Would be great to add coverage for at least: URL skipping, require.resolve resolution, and workspace name resolution.
|
Addressed all review feedback in https://github.com/advancedresearcharray/mex/commit/72efcdf (branch ChangesHIGH — pnpm workspace support
MEDIUM — URL regex
MEDIUM —
LOW — Tests
All 87 tests pass; @adelin-b — could you cherry-pick or merge |
|
Review feedback is implemented on branch @adelin-b — to update this PR in one step, merge adelin-b/mex#1 (review fixes into your All four review items are covered:
|
|
Updated commit (no co-author trailers): advancedresearcharray/mex@fdbb507. Merge via adelin-b/mex#1 when ready. |
|
All four review items are addressed in commit Review responsesHIGH — pnpm workspace support MEDIUM — URL regex MEDIUM — LOW — Tests All 87 tests pass; Updating this PRThe fix branch lives on git fetch https://github.com/advancedresearcharray/mex.git fix/skip-scoped-packages-and-urls
git push https://github.com/adelin-b/mex.git FETCH_HEAD:fix/skip-scoped-packages-and-urlsAlternatively, @adelin-b can merge adelin-b/mex#1, or merge #68 which contains the same commits ready for review. |
Skip URL-valued claims and resolve scoped workspace package aliases: npm/yarn/bun workspaces plus a pnpm-workspace.yaml fallback, anchored via createRequire(noop.js). Builds on #17 by adelin-b; review fixes and tests by advancedresearcharray.
|
Thanks @adelin-b for the original fix here. The four review items (pnpm-workspace.yaml support, broader URL filter, canonical createRequire anchor, and tests) were all addressed and just merged as #68, which builds directly on your branch and commits. Closing this as superseded by #68 - the feature is now on main. Appreciate the contribution. |
Summary
MISSING_PATHerrors. In monorepos, references like@acme/uior@acme/shared/utilsin scaffold files are valid import aliases, not filesystem paths.http://,https://) which were being treated as filesystem paths because they contain/.How it works
require.resolvefirst — tries Node's built-in module resolution for installed npm packages (handlesnode_modulespackages regardless of hoisting strategy)package.jsonworkspacesfield, globs each pattern, and collects thenamefrom each workspace'spackage.json. This handles package managers like bun that don't symlink all workspaces intonode_modules/http://andhttps://prefixed values before path checkingExample
A monorepo with this structure:
Previously, scaffold references like
@acme/ui/buttonor@acme/shared/typeswould trigger:Now they resolve correctly via the workspace name lookup.
Test plan
@scope/*aliases now resolve correctlynode_modulesstill resolve viarequire.resolve