wire augmentation, fix cli version#10
wire augmentation, fix cli version#10NavehBrenner wants to merge 1 commit intoplumbus-framework:mainfrom
Conversation
markkr125
left a comment
There was a problem hiding this comment.
Hi I asked for an AI review, i think the robot makes sense.
PR #10 Review
PR Summary: Two changes bundled together:
- Auto-add
.plumbus/generatedto tsconfig.jsonincludeafter runningplumbus generate - Read CLI version dynamically from package.json instead of hardcoding
'0.1.0'
Overall Assessment: Needs changes before merge
Change 1: ensureTsconfigIncludesGenerated — Mostly Good
What works well:
- Clean, well-tested function with good edge cases (missing file, no
includearray, already present, happy path) - Properly exports from the barrel files
- Uses existing utils (
exists,readJson,writeFile,warn) instead of reimplementing - Documentation in commands.md and data-layer.md updated to match
Issues:
-
tsconfigcomments will be destroyed —JSON.stringify(tsconfig, null, 2)at generate.ts line 510 will silently strip any comments (JSONC) that consumer tsconfig files commonly have. This is a data loss risk. Consider using a JSONC-aware parser (e.g.jsonc-parserfrom thejsonc-parsernpm package, orstrip-json-comments+ careful round-tripping), or at minimum warn the user that comments were removed. -
Trailing newline loss —
JSON.stringifydoesn't add a trailing newline. Most tsconfigs end with\n. The rewrite will remove it, causing noisy diffs for the consumer. Add+ '\n'to the write. -
Monorepo path is too narrow — The monorepo branch only patches
backend/tsconfig.json:const tsconfigPaths = monorepo.isMonorepo ? [resolvePath('backend', 'tsconfig.json')] : [resolvePath('tsconfig.json')];
What about the frontend tsconfig, or a project that uses
apps/instead ofbackend/? At minimum, this should also handle the frontend package sinceplumbus.d.ts/entity-types.tsare written tolibs/shared/types/in monorepo mode. Consider using the monorepo layout info to derive paths rather than hardcodingbackend/. -
No
includearray = silent no-op — If a consumer's tsconfig has noincludekey at all (relying on the default "everything" behavior), the function returnsfalsesilently. This means the generated types might already be picked up — so this may be fine — but a log message likeinfo('tsconfig.json has no include array — generated types should be picked up automatically')would help avoid user confusion. -
Test isolation concern — The tests use real filesystem
/tmp/plumbus-tsconfig-test-${Date.now()}withafterEachcleanup. This is fine functionally but:- Uses
afterEach(notafterAll) which is correct for cleanup per test - Missing
beforeEach— thetmpDiris created inside each test, but if a test fails beforemkdirSync,afterEachcouldrmSynca non-existent dir (theexistsSyncguard handles this, so it's OK) - Would be slightly cleaner to use Vitest's built-in
vi.stubEnv/ tmp dir utilities, but not a blocker
- Uses
Change 2: Dynamic CLI Version — Has a Bug Risk
The change:
function getVersion(): string {
const __dirname = dirname(fileURLToPath(import.meta.url));
const pkgPath = resolve(__dirname, '..', '..', 'package.json');
const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8')) as { version: string };
return pkg.version;
}Issues:
-
Path resolution is fragile after build —
import.meta.urlresolves relative to the executing file. After TypeScript compilation, cli.ts ends up indist/cli/cli.js(two levels deep indist/), soresolve(__dirname, '..', '..')points to the package root. BUT — if the build output structure ever changes (e.g. flatdist/), or if this file is bundled, the path breaks silently. A safer approach would be to import the version from package.json using Node'screateRequire, or use a build-time replacement. -
No error handling — If
readFileSyncorJSON.parsethrows (corrupted install, wrong path), the CLI crashes with an unhandled error before any command runs. Wrap in try/catch with a fallback like'unknown'. -
Test is too permissive — The new test:
expect(program.version()).toMatch(/^\d+\.\d+\.\d+/);
This matches any semver-ish string but doesn't verify it actually reads from package.json. It could pass even if
getVersion()returned a random hardcoded version. Consider reading package.json in the test and asserting equality.
Missing from the PR
-
No barrel export for
ensureTsconfigIncludesGeneratedin public API — The function is exported from src/cli/index.ts and src/index.ts. This is a framework-internal utility — does it really need to be on the public SDK surface (src/index.tsTIER 2)? If it's only used insideregisterGenerateCommand, it should stay internal to the CLI module and not be re-exported from the top-level barrel. -
Commit hygiene — Three commits ("all", "no need for it by default", "fix cli version command") have unclear messages. Should be squashed into a single commit with a descriptive message before merge.
Summary Table
| # | Severity | Issue |
|---|---|---|
| 1 | High | JSONC comments destroyed on tsconfig rewrite |
| 2 | Low | Missing trailing newline on rewritten tsconfig |
| 3 | Medium | Monorepo only patches backend/tsconfig.json, not frontend |
| 4 | Low | Silent no-op when include array is absent |
| 5 | Nit | Test filesystem approach is fine but could be cleaner |
| 6 | Medium | import.meta.url path resolution fragile after build |
| 7 | Medium | No error handling in getVersion() — CLI crashes if path wrong |
| 8 | Low | Version test too permissive |
| 9 | Low | Over-exporting internal utility to public API |
| 10 | Nit | Commit messages need cleanup |
8368c30 to
cde2b2a
Compare
changes
types
add plumbus.d.ts type gumentation to tsconfig include paths on "plumbus generate" cli command
version
remove hard coded 1.0.0 cli version and dynamically fetch if from the package json to display the current version of plumbus installed