refactor: update resolveProjectRoot to support source paths in addition to dist paths#152
refactor: update resolveProjectRoot to support source paths in addition to dist paths#152besinai wants to merge 1 commit intonexu-io:mainfrom
Conversation
mrcfps
left a comment
There was a problem hiding this comment.
@besinai thanks for tightening up source-path handling here 🙏 This is headed in the right direction, but I found one runtime path shape that this change accidentally breaks. Could you handle that case and add coverage for it? 😊
| // In both cases we need to go 2 levels above the daemon package dir. | ||
| const basename = path.basename(moduleDir); | ||
| const daemonDir = | ||
| basename === 'dist' || basename === 'src' |
There was a problem hiding this comment.
This now treats any src basename as apps/daemon/src, but the sidecar build can emit/import this file from apps/daemon/dist/src/server.js (tsconfig.sidecar.json sets rootDir: "." and includes both sidecar/**/*.ts and src/**/*.ts). In that runtime, moduleDir is .../apps/daemon/dist/src, so path.dirname(moduleDir) becomes .../apps/daemon/dist and path.resolve(daemonDir, '../..') resolves to .../apps instead of the repo root. Could you handle dist/src explicitly (or walk upward to the daemon package root/package.json) and add tests for both apps/daemon/src and apps/daemon/dist/src? 🚧
lefarcen
left a comment
There was a problem hiding this comment.
Agreeing with @mrcfps on the dist/src sidecar path issue — that's the critical gap here.
A few additional observations to help close this out:
Correctness / Edge cases:
- The current fix handles
srcanddistat the daemon package level, but as @mrcfps noted, sidecar'sdist/srcnesting breaks the../..assumption. - Consider: what if someone runs
tsxfrom a symlinked path, or__dirnamepoints tonode_modules/.pnpm/...? A robust approach might walk upward looking forpackage.jsonwith"name": "@open-design/daemon"instead of hardcoding depth.
Test coverage:
-
This function is foundational (every daemon boot depends on it), but there are no tests exercising it. Adding a small unit test suite covering:
apps/daemon/dist/server.js(compiled, main entry)apps/daemon/src/server.ts(tsx dev mode)apps/daemon/dist/src/server.js(sidecar build)- Edge case:
apps/daemon/dist/sidecar/xxx.js(if sidecar has its own subdirs)
…would prevent future regressions and make the intent crystal-clear.
Architecture / Design:
- The inline comment you added is helpful! But since this logic now has 3+ branches, consider extracting it into a helper with a docstring explaining the runtime shapes. Something like:
/** * Resolves the monorepo root from the daemon's module directory. * Handles: * - Compiled main build: apps/daemon/dist/server.js * - Dev (tsx) mode: apps/daemon/src/server.ts * - Sidecar build: apps/daemon/dist/src/server.js (tsconfig.sidecar.json) */ export function resolveProjectRoot(moduleDir: string): string { ... }
No blocking issues beyond what @mrcfps already flagged. Once you handle dist/src + add tests, this will be solid. Thanks for tackling this! 🙏
|
Hi @besinai! Just checking in on this PR — it's been about 41h since the last update, and there are now merge conflicts with main. No rush if you're still working on it. If you'd like a hand, just reply here — happy to walk through the rebase strategy or clarify any of the review points. If you've moved on, no worries — feel free to close the PR. Thanks for the contribution! |
Design Systems and Theme loading fixed