refactor: use nested packages in vitest alias.#519
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
🌏 Preview DeploymentsNote No deployable apps affected by changes in this PR. Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
There was a problem hiding this comment.
Pull request overview
Refactors the Vitest alias generation to support collecting aliases from nested workspace package roots (notably packages/pipelines) so tests can resolve local package sources consistently across the monorepo.
Changes:
- Generalize alias path helpers to accept a configurable package root.
- Add
collectAliasesFromRoot()and merge alias maps frompackagesandpackages/pipelines. - Keep explicit
#test-utils/*aliases, updated to use the new helper signature.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return dirs.reduce<Record<string, string>>( | ||
| (acc, pkg) => { | ||
| let scope = "@ucdjs"; | ||
|
|
||
| if (SCOPE_OVERRIDES[pkg]) { | ||
| scope = SCOPE_OVERRIDES[pkg]; | ||
| } | ||
|
|
||
| acc[`${scope}/${pkg}`] = alias(pkg); | ||
| acc[`${scope}/${pkg}`] = alias(root, pkg); | ||
| return acc; |
There was a problem hiding this comment.
Alias keys are currently derived from the directory name (pkg). For packages/pipelines/*, directory names are pipeline-* but the actual package names are @ucdjs/pipelines-* (see those packages’ package.json name fields), so imports like @ucdjs/pipelines-core won’t match any generated alias and tests may resolve the wrong entrypoint or fail when dist isn’t built. Consider deriving the alias key from each package’s package.json name (and falling back to the dir name only if missing), rather than ${scope}/${pkg}.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vitest.aliases.ts (1)
16-19: Two minor improvements for robustness and efficiency incollectAliasesFromRoot.a) No guard against a non-existent root. If
packages/pipelinesdoesn't exist on disk,readdirSync(rootDir)throwsENOENTand breaks every test run with an opaque error. An early existence check keeps the failure message clear (or allows silently returning an empty record).b)
readdirSyncwithoutwithFileTypes. Leveraging thewithFileTypesoption efficiently retrieves additional file information, reducing the need for multiple system calls. Using{ withFileTypes: true }lets you pre-filter ondirent.isDirectory()before callingexistsSync, avoiding probing non-directory entries entirely.♻️ Suggested refactor
function collectAliasesFromRoot(root: string): Record<string, string> { const rootDir = fileURLToPath(new NodeURL(`./${root}`, import.meta.url)); - const dirs = readdirSync(rootDir) - .filter((dir) => existsSync(pkgRoot(root, dir) + "/package.json")); + if (!existsSync(rootDir)) return {}; + const dirs = readdirSync(rootDir, { withFileTypes: true }) + .filter((dirent) => dirent.isDirectory() && existsSync(pkgRoot(root, dirent.name) + "/package.json")) + .map((dirent) => dirent.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.aliases.ts` around lines 16 - 19, collectAliasesFromRoot currently calls readdirSync(rootDir) without checking that rootDir exists and without using withFileTypes, which can cause ENOENT or extra stat calls; first add an early guard using existsSync(rootDir) and return an empty record (or a clear error) if missing, then call readdirSync(rootDir, { withFileTypes: true }) and filter dirents by dirent.isDirectory() before mapping to names and checking existsSync(pkgRoot(root, dirName) + "/package.json"); keep the rest of the logic in collectAliasesFromRoot, still using fileURLToPath(new NodeURL(...)) to build rootDir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vitest.aliases.ts`:
- Around line 35-37: The current aliases construction spreads two maps so keys
from collectAliasesFromRoot("packages/pipelines") silently overwrite those from
collectAliasesFromRoot("packages"); change this to perform a guarded merge: call
collectAliasesFromRoot for each root into separate objects, iterate the second
object's keys and check for existing keys in the first (aliases) and either
throw/log a descriptive error or handle duplicates explicitly before assigning;
update the code around the aliases export and reference collectAliasesFromRoot
and aliases so collisions are detected and reported instead of overwritten.
---
Nitpick comments:
In `@vitest.aliases.ts`:
- Around line 16-19: collectAliasesFromRoot currently calls readdirSync(rootDir)
without checking that rootDir exists and without using withFileTypes, which can
cause ENOENT or extra stat calls; first add an early guard using
existsSync(rootDir) and return an empty record (or a clear error) if missing,
then call readdirSync(rootDir, { withFileTypes: true }) and filter dirents by
dirent.isDirectory() before mapping to names and checking
existsSync(pkgRoot(root, dirName) + "/package.json"); keep the rest of the logic
in collectAliasesFromRoot, still using fileURLToPath(new NodeURL(...)) to build
rootDir.
| export const aliases = { | ||
| ...collectAliasesFromRoot("packages"), | ||
| ...collectAliasesFromRoot("packages/pipelines"), |
There was a problem hiding this comment.
Silent alias collision when packages and packages/pipelines share a package name.
Because collectAliasesFromRoot("packages/pipelines") is spread after collectAliasesFromRoot("packages"), any package whose name exists in both directories will have its packages-level alias silently overwritten by the packages/pipelines one with no warning. For example, packages/foo → @ucdjs/foo would be replaced by packages/pipelines/foo → @ucdjs/foo.
If name collisions are guaranteed to be impossible by repo convention, this is fine as-is. Otherwise, consider adding a runtime collision check:
🛡️ Optional defensive guard
+function mergeAliases(...records: Record<string, string>[]): Record<string, string> {
+ const merged: Record<string, string> = {};
+ for (const record of records) {
+ for (const [key, value] of Object.entries(record)) {
+ if (key in merged && merged[key] !== value) {
+ throw new Error(`Alias collision for "${key}": "${merged[key]}" vs "${value}"`);
+ }
+ merged[key] = value;
+ }
+ }
+ return merged;
+}
+
export const aliases = {
- ...collectAliasesFromRoot("packages"),
- ...collectAliasesFromRoot("packages/pipelines"),
+ ...mergeAliases(
+ collectAliasesFromRoot("packages"),
+ collectAliasesFromRoot("packages/pipelines"),
+ ),
"#test-utils/msw": alias("packages", "test-utils") + "/msw.ts",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const aliases = { | |
| ...collectAliasesFromRoot("packages"), | |
| ...collectAliasesFromRoot("packages/pipelines"), | |
| function mergeAliases(...records: Record<string, string>[]): Record<string, string> { | |
| const merged: Record<string, string> = {}; | |
| for (const record of records) { | |
| for (const [key, value] of Object.entries(record)) { | |
| if (key in merged && merged[key] !== value) { | |
| throw new Error(`Alias collision for "${key}": "${merged[key]}" vs "${value}"`); | |
| } | |
| merged[key] = value; | |
| } | |
| } | |
| return merged; | |
| } | |
| export const aliases = { | |
| ...mergeAliases( | |
| collectAliasesFromRoot("packages"), | |
| collectAliasesFromRoot("packages/pipelines"), | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.aliases.ts` around lines 35 - 37, The current aliases construction
spreads two maps so keys from collectAliasesFromRoot("packages/pipelines")
silently overwrite those from collectAliasesFromRoot("packages"); change this to
perform a guarded merge: call collectAliasesFromRoot for each root into separate
objects, iterate the second object's keys and check for existing keys in the
first (aliases) and either throw/log a descriptive error or handle duplicates
explicitly before assigning; update the code around the aliases export and
reference collectAliasesFromRoot and aliases so collisions are detected and
reported instead of overwritten.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🔗 Linked issue
📚 Description
Summary by CodeRabbit