Conversation
Move type-resolution primitives (project creation, temporary type-alias injection/cleanup, type text resolution with enum expansion) behind a TsHost interface implemented by TsMorphHost. Behavior is unchanged; this narrows the seam a future tsgo/Corsa-API-based host would need to fill per issue #200.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/extractor.ts`:
- Line 166: Wrap the imported/local extraction and post-processing phase
following host.ensureNormalizeType(sourceFile) in a try/finally, ensuring the
existing cleanup that removes __Normalize from the shared SourceFile always
runs. Preserve the current extraction behavior and error propagation while
guaranteeing cleanup when any resolution or processing step throws.
In `@src/core/ts-morph-host.ts`:
- Around line 25-32: Track the exact alias nodes inserted by ensureNormalizeType
and resolveTypes instead of identifying ownership through fixed-name lookups.
Store each newly added alias node and have cleanupNormalizeType remove only the
tracked node, preserving any pre-existing __Normalize declaration and applying
the same ownership-safe cleanup to all temporary aliases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb9cd4f4-c7e9-4c04-94f1-373683527ba4
📒 Files selected for processing (4)
.changeset/quiet-plums-relax.mdsrc/core/extractor.tssrc/core/ts-host.tssrc/core/ts-morph-host.ts
📜 Review details
🔇 Additional comments (4)
src/core/ts-host.ts (1)
1-51: LGTM!src/core/ts-morph-host.ts (1)
1-23: LGTM!Also applies to: 54-133
src/core/extractor.ts (1)
1-8: LGTM!Also applies to: 31-46, 130-144
.changeset/quiet-plums-relax.md (1)
1-5: LGTM!
|
|
||
| // Inject __Normalize once for the main source file | ||
| this.ensureNormalizeType(sourceFile); | ||
| this.host.ensureNormalizeType(sourceFile); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guarantee main-source cleanup with finally.
If any local resolution or post-processing throws, Line 271 is skipped and __Normalize remains attached to the shared SourceFile. Wrap the imported/local extraction phase in try/finally.
Proposed structure
this.host.ensureNormalizeType(sourceFile);
-
-// Extract imported and local schemas
-this.host.cleanupNormalizeType(sourceFile);
+try {
+ // Extract imported and local schemas
+} finally {
+ this.host.cleanupNormalizeType(sourceFile);
+}Also applies to: 168-271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/extractor.ts` at line 166, Wrap the imported/local extraction and
post-processing phase following host.ensureNormalizeType(sourceFile) in a
try/finally, ensuring the existing cleanup that removes __Normalize from the
shared SourceFile always runs. Preserve the current extraction behavior and
error propagation while guaranteeing cleanup when any resolution or processing
step throws.
Comment on lines
+25
to
+32
| ensureNormalizeType(sourceFile: SourceFile): void { | ||
| if (!sourceFile.getTypeAlias("__Normalize")) { | ||
| sourceFile.addStatements([NORMALIZE_TYPE_DEFINITION]); | ||
| } | ||
| } | ||
|
|
||
| cleanupNormalizeType(sourceFile: SourceFile): void { | ||
| sourceFile.getTypeAlias("__Normalize")?.remove(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find existing collision coverage and all fixed synthetic-name usages.
rg -n -C3 '__Normalize|__TempInput|__TempOutput|__TempExplicit' src test tests 2>/dev/null || trueRepository: toiroakr/zinfer
Length of output: 7889
Track the injected aliases, not the fixed names. cleanupNormalizeType() can delete a pre-existing __Normalize declaration because ownership is inferred from getTypeAlias("__Normalize"); the same fixed-name lookup pattern applies to the temporary aliases in resolveTypes. Keep the exact inserted nodes and remove those nodes directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/ts-morph-host.ts` around lines 25 - 32, Track the exact alias nodes
inserted by ensureNormalizeType and resolveTypes instead of identifying
ownership through fixed-name lookups. Store each newly added alias node and have
cleanupNormalizeType remove only the tracked node, preserving any pre-existing
__Normalize declaration and applying the same ownership-safe cleanup to all
temporary aliases.
Full cutover from ts-morph to TypeScript's native tsgo/Corsa API (@typescript/native-preview), completing the migration started with the TsHost abstraction. ts-morph is dropped as a dependency entirely. - TsgoHost implements TsHost by resolving temporary type aliases through a virtual-FS overlay, since the Corsa API is read-only at the AST level. Always marks the relevant root file(s) as changed on every project open, since reusing the same virtual tsconfig path across calls with different overlay content otherwise leaves the server serving a stale parse. - SchemaDetector, GetterResolver, ImportResolver, BrandDetector, and SchemaReferenceAnalyzer are reimplemented against the Corsa ast/checker API. ImportResolver's cross-file resolution now goes through Checker.getSymbolAtLocation (real module resolution) instead of ts-morph's getModuleSpecifierSourceFile, incidentally fixing a known limitation resolving named imports through an intermediate re-export index file. - When a tsconfig.json is supplied, its resolved CompilerOptions are sanitized before being re-embedded in the virtual tsconfig: enum fields come back as raw numbers and lib entries as resolved filenames (both invalid tsconfig.json syntax), and rootDir/outDir/types/typeRoots are tied to the original project's directory layout rather than ours. - @typescript/native-preview moves from a dev-only dependency (used only for the tsgo typecheck script) to a runtime dependency. A handful of generated snapshots changed to reflect the new checker's member ordering: object/mapped types print in source declaration order, and z.enum([...]) string literal unions print alphabetically. SchemaDetector and BrandDetector are part of the public API and now operate on a @typescript/native-preview SourceFile instead of a ts-morph one. ZodTypeExtractor's public surface and the top-level extractZodTypes/ extractAndFormat/extractAllSchemas helpers are unaffected.
toiroakr
changed the title
Neither is documented in the README's Library API section or used anywhere outside the package itself. Now that they operate on a @typescript/native-preview SourceFile, there's no public, supported way to construct one to call them with (TsgoHost isn't exported either), so keeping them exported would be a half-usable API. Everything they exposed is already available through ZodTypeExtractor's output (ExtractResult#isExported/#brands) and #getSchemaNames. Also fixes a stale README line still crediting ts-morph.
Ship the ts-morph -> tsgo/Corsa API migration as a prerelease under the next npm dist-tag rather than straight to latest, given tsgo is still a dev-preview build with known checker-behavior divergences from mainline TypeScript (see PR discussion on #200). Run 'changeset pre exit' once this has baked long enough to promote to latest. Also drop json/jsonc from oxlint's pre-commit glob: oxlint has no standalone JSON linting support and errors out with 'no files found' when a commit touches only json/jsonc files (as this one does), a pre-existing gap this changeset file happened to trip. oxfmt still formats those extensions.
Entering pre mode on main would have swept every subsequent changeset (not just this one) into next-tagged prereleases, blocking latest releases entirely until someone remembered to run pre exit. Repo-wide pre-release mode is the wrong tool here; a one-off snapshot release (changeset version --snapshot / changeset publish --tag) is the right way to ship just this change under a non-latest tag without touching the normal release queue.
Add a release workflow scoped to the ts7 branch only (release-ts7.yml), separate from main's release.yml, and enter changesets pre-release mode (tag: next) so pushes to ts7 publish -next.N prereleases without touching main's normal release cadence or the latest dist-tag. Before merging ts7 into main: run 'pnpm changeset pre exit' and delete release-ts7.yml, so main never inherits pre-release mode.
Full engine swap plus removal of public SchemaDetector/BrandDetector exports warrants a major bump rather than continuing to ride the 0.x minor-for-breaking-changes convention.
chore: release (ts7 preview) (next)
The dedicated release-ts7.yml workflow published under an npm OIDC trusted-publishing identity npm doesn't recognize (trusted publishing is scoped to this exact workflow file by name), causing a 404 on publish. Simpler fix: just add ts7 to release.yml's own branch triggers instead of introducing a second workflow file. Remove the ts7 branch trigger once ts7 is merged into main.
…tructing options Replace parseConfigFile()-based CompilerOptions reconstruction with serving a patched copy of the real tsconfig.json at its own path: parse it with jsonc-parser (tolerating comments/trailing commas, which real tsconfig.json files do support - verified empirically against tsc, tsgo, and parseConfigFile()) and append the target file to `files`, leaving everything else - including `extends` - untouched. This sidesteps two problems in the previous approach: - parseConfigFile() returns fully-resolved CompilerOptions (numeric enum values, resolved lib filenames) that isn't valid tsconfig.json syntax to write back, requiring a hand-rolled reconstruction. - Serving the config from a synthetic path elsewhere (e.g. os.tmpdir()) broke relative extends/typeRoots resolution, which the earlier typeRoots workaround was compensating for. Since the patched config is served at the *same* absolute path as the original, tsgo resolves extends, typeRoots, and every other config-relative path exactly as it would for the real project - no special-casing needed. Verified against a real extends chain with comments in the base config. Only the no-tsconfig-given default path still serves a synthetic config (there's no real file to preserve resolution semantics for).
chore: release (next)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full cutover from
ts-morphto TypeScript's native tsgo/Corsa API (@typescript/native-preview) for issue #200, building on theTsHostabstraction from the earlier commit in this PR.TsgoHostimplementsTsHostby resolving temporary type aliases through a virtual-FS overlay (the Corsa API is read-only at the AST level, so there's no liveSourceFileto mutate). Each call always marks its target root file(s) as changed on the underlying project, since reusing the same virtual tsconfig path across calls with different overlay content otherwise leaves the server serving a stale parse.SchemaDetector,GetterResolver,ImportResolver,BrandDetector, andSchemaReferenceAnalyzerare reimplemented against the Corsaast/checker API.ImportResolver's cross-file resolution now goes throughChecker.getSymbolAtLocation(real module resolution) instead of ts-morph'sgetModuleSpecifierSourceFile- which incidentally fixes a known limitation resolving named imports through an intermediate re-export index file (see the updated test inimport-resolver.test.ts).tsconfig.jsonis supplied, its resolvedCompilerOptionsare sanitized before being re-embedded into our virtual tsconfig: enum fields come back from the API as raw numbers andlibentries as resolved filenames (both invalidtsconfig.jsonsyntax), androotDir/outDir/types/typeRootsare tied to the original project's directory layout rather than ours.@typescript/native-previewmoves from a dev-only dependency (previously used just for thetsgotypecheck script) to a runtime dependency.ts-morphis dropped entirely.SchemaDetector/BrandDetectorare no longer exported from the package - they weren't documented in the README's Library API section or used anywhere outside the package, and now that they operate on a@typescript/native-previewSourceFile, there's no supported way to construct one to call them with (TsgoHostisn't exported either). Everything they exposed is already available throughExtractResult#isExported/#brandsandZodTypeExtractor#getSchemaNames.Behavior notes
A handful of generated snapshots changed to reflect the new checker's (arguably more correct) member ordering: object/mapped types now print in source declaration order, and
z.enum([...])-derived string literal unions print alphabetically.ZodTypeExtractor's public methods and the top-levelextractZodTypes/extractAndFormat/extractAllSchemashelpers are unaffected.Verified the CLI end-to-end against tsconfig-based projects, cross-file imports, subpath imports (package.json
importsfield), and multi-file glob processing.Release status
@typescript/native-previewis still a7.1.0-devpreview build, and we already found real checker-behavior divergences from ts-morph's classic checker during this migration (see above), so this isn't going straight tolatest.Currently published as
zinfer@1.0.0-next.0under the npmnextdist-tag (npm install zinfer@next), via changesets pre-release mode (.changeset/pre.json, tagnext) scoped to thets7branch.release.ymltemporarily also triggers on push tots7so publishing doesn't need a separate npm Trusted Publisher entry.main's normal releases and thelatestdist-tag (currently0.2.5) are untouched.Before merging into
mainpnpm changeset pre exitonts7ts7branch trigger from.github/workflows/release.yml(see comment left in the file)main/latest(currently previewing as1.0.0)nextwas being tried outRelated Issues
Closes #200