fix: fromTypes response parsing — numeric keys, type alias inlining, import() resolution#329
fix: fromTypes response parsing — numeric keys, type alias inlining, import() resolution#329alexng353 wants to merge 10 commits into
Conversation
…ing, route section trimming Three bugs prevented fromTypes from correctly parsing auto-generated .d.ts declarations into OpenAPI response schemas: 1. numberKey regex matched digits inside identifiers (e.g. v4: → v"4":) causing TypeBox to produce invalid schemas. Fixed with lookbehind to only match standalone numeric keys. 2. Type alias references (e.g. User) produced unresolvable $refs since TypeBox has no type definitions context. Added extractTypeAliases() and inlineTypeReferences() to substitute type bodies before parsing. 3. Trailing generic params after the routes map were included in extraction, producing many spurious objects. Added brace-depth scanning to trim at the routes param boundary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… trimming Tests cover all three bug fixes: - numberKey regex: digits inside identifiers (v4) are preserved while standalone numeric keys (200, 404) are still quoted - extractTypeAliases / inlineTypeReferences: type alias extraction from declaration preambles and substitution into route schemas - declarationToJSONSchema with typeAliases param: end-to-end type alias inlining producing correct JSON Schema for User, ErrorResponse, etc. - Route section trimming: extractRootObjects returns correct count Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TypeBox cannot resolve cross-module import("...").TypeName references,
producing invalid schemas. These now get replaced with `unknown` before
parsing, which maps to `{}` in JSON Schema (any value).
Tests added for import() stripping and a full Drizzle-derived User type
with nullable fields, string literal unions, optional nested objects,
and array types.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…odule resolution
Instead of replacing import("...").TypeName with `unknown`, the plugin
now resolves the import path using ts.resolveModuleName() (which handles
tsconfig paths, package.json exports, workspace packages, etc.), reads
the source file, extracts the type alias, and inlines it.
This means Drizzle ORM types and other cross-package types get full
JSON Schema output instead of degrading to `{}`.
Also:
- Strip comments from extracted type alias bodies (// and /* */)
- Externalize typescript from the bundle to avoid 9MB dist
- Use source file path as resolution context for workspace symlinks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lback resolveImportedTypes now throws a clear error if the typescript package cannot be loaded, rather than silently falling back to manual path heuristics. TypeScript's module resolution is the only reliable way to resolve cross-module import() references in monorepos and workspace packages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBuild config and manifest updated: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.ts`:
- Around line 11-12: The build config marks typescript as external (external:
['typescript']) so it isn't bundled, but runtime code uses
ts.resolveModuleName() via import() so consumers must provide TypeScript; update
package.json to declare "typescript" as a peerDependency (or move to
dependencies if you intend to bundle/own it) and bump the package.json version
if required, ensuring the package.json fields "peerDependencies" (add
"typescript": "<minimum-supported-version>") reflect the same runtime
requirement referenced by external in build.ts and the ts.resolveModuleName()
usage.
typescript is externalized in the build (not bundled) but required at
runtime by resolveImportedTypes() via require('typescript'). Declaring
it as a peerDependency ensures package managers surface a warning if
it's missing.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
89-92:typescriptpeer dependency should be marked optional viapeerDependenciesMeta.
typescriptis only needed at runtime forfromTypes()/resolveImportedTypes()— users who don't call that feature have no need for it. As a non-optionalpeerDependency, every consumer of this package will receive peer-dep warnings (npm) or hard errors (pnpm) if they don't have TypeScript installed, even though it's entirely irrelevant to their usage.Marking a peer dependency as optional ensures npm will not emit a warning if the package is not installed on the host. With Bun, running
bun installwill install peer dependencies by default, unless marked optional inpeerDependenciesMeta.🛠️ Proposed fix
"peerDependencies": { "elysia": ">= 1.4.0", "typescript": ">= 5.0.0" }, + "peerDependenciesMeta": { + "typescript": { + "optional": true + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 89 - 92, The package declares "typescript" under "peerDependencies" causing warnings/errors for consumers who don't need it; add a "peerDependenciesMeta" entry marking "typescript" as optional so package managers won't warn/install it by default. Update package.json to include a peerDependenciesMeta object with "typescript": { "optional": true } alongside the existing "peerDependencies" to make the typescript peer dep optional (this targets the "peerDependencies" and "typescript" symbols in package.json).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 93-95: The package currently lists "@sinclair/typebox" in
"dependencies" which causes its types (e.g., TProperties used in the exported
withHeaders function) to be embedded in the public API and can cause
duplicate-type conflicts for consumers; move "@sinclair/typebox" from
"dependencies" to "peerDependencies" and update its version string to a
patch-pinned form "~0.34.0" so consumers provide a compatible TypeBox instance
and TypeScript resolution remains consistent (ensure package.json entries for
"dependencies" and "peerDependencies" are updated accordingly and keep
withHeaders/TProperties usage unchanged).
---
Nitpick comments:
In `@package.json`:
- Around line 89-92: The package declares "typescript" under "peerDependencies"
causing warnings/errors for consumers who don't need it; add a
"peerDependenciesMeta" entry marking "typescript" as optional so package
managers won't warn/install it by default. Update package.json to include a
peerDependenciesMeta object with "typescript": { "optional": true } alongside
the existing "peerDependencies" to make the typescript peer dep optional (this
targets the "peerDependencies" and "typescript" symbols in package.json).
typebox types (TProperties, TObject) are exposed in the public API via the exported withHeaders function. Having typebox as a hard dependency can cause duplicate instances when the consumer's elysia (which also peer-depends on typebox) resolves a different copy, leading to type incompatibilities. Moving to peerDependencies ensures a single shared instance, matching elysia's own approach. Also added @sinclair/typebox to the tsup external list so it is not bundled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
92-92: Marktypescriptas optional inpeerDependenciesMeta.
typescriptis only required by consumers who use thefromTypes()API. Basic Swagger/OpenAPI users don't need TypeScript as a runtime peer. WithoutpeerDependenciesMeta, package managers will warn all consumers about a missingtypescriptpeer, even those who never invokefromTypes().♻️ Proposed change
"peerDependencies": { "@sinclair/typebox": ">= 0.34.0 < 1", "elysia": ">= 1.4.0", "typescript": ">= 5.0.0" }, + "peerDependenciesMeta": { + "typescript": { + "optional": true + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 92, Add a peerDependenciesMeta entry to package.json to mark TypeScript as optional so package managers won't warn consumers who don't need it; specifically, under the existing peerDependencies/ "typescript": ">= 5.0.0" ensure there is a peerDependenciesMeta object with "typescript": { "optional": true } (preserving the same version spec in peerDependencies) so only consumers using the fromTypes() API need to install TypeScript.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 92: Add a peerDependenciesMeta entry to package.json to mark TypeScript
as optional so package managers won't warn consumers who don't need it;
specifically, under the existing peerDependencies/ "typescript": ">= 5.0.0"
ensure there is a peerDependenciesMeta object with "typescript": { "optional":
true } (preserving the same version spec in peerDependencies) so only consumers
using the fromTypes() API need to install TypeScript.
- Add flattenNestedIntersections() to distribute parent structure over
inner `& {}` boundaries from multi-route Elysia plugins
- Add extractGenericParam() for brace-aware comma parsing of Elysia's
generic parameters (replaces fragile `}, {` slicing)
- Bump elysia devDep to 1.4.25
- Update test expectations for cleaner route output (no derive/resolve
noise)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
|
Hello @SaltyAom , just a heads up, I've been using my fork of elysia-openapi in production since this PR was opened. I've not experienced any bugs myself. If you need any information regarding this PR, I'm afraid I will not be able to answer before reviewing the code again myself, as I haven't taken a look at this in over a month. Please be patient with me for any inquiries you have, work is extremely heavy for me at the moment, but I will get to anything you need as soon as possible. |
|
yes please, have been waiting for this! @SaltyAom |
|
@alexng353 can you update to latest version from upstream |
|
Used this branch as base for a downstream fork: https://github.com/0-don/elysia-openapi-fork/tree/fix/fromtypes-response-parsing-dist (review commits on |
|
I was able to inline my refs myself via a util helper export interface Test {
name: string;
age: number;
}
.get('/test/omfg3', () : Prettify<Test> => {
return {
name: 'Hello',
age: 4
} as Test;
})
```
|
4 participants
Summary
fromTypes()fails to generate correct OpenAPI response schemas in several common scenarios. This PR fixes three parsing bugs and adds support for cross-module type resolution:Numeric key regex — The regex
(\d+):/gthat quotes status code keys (e.g.200:->"200":) also matches digits inside identifiers likev4:, producingv"4":which TypeBox rejects. Fixed with a lookbehind:/(?<=^|[{;,\s])(\d+):/g.Type alias inlining — When TypeScript emits type aliases in
.d.ts(e.g.type User = { id: string; ... }) and references them in route responses, TypeBox produces unresolvable$refnodes. Fixed by extracting type aliases from the declaration and inlining them before TypeBox parsing.Route section trimming —
extractRootObjectscould include trailing content from adjacent type declarations. Fixed with brace-depth scanning to extract only the route object.Cross-module
import()resolution — When tsc emitsimport("@some/package").TypeNamereferences (common with Drizzle ORM and other codegen), these are not parseable by TypeBox. Fixed by using TypeScript'sts.resolveModuleName()to locate the source file, extract the type definition, and inline it. This handles monorepo workspace packages, path aliases, andexportsmaps correctly.Build config — Added
typescripttoexternalin tsup config to prevent bundling it (was causing ~9.7MB output).Bugs this fixes
Given an Elysia app like:
Before: The OpenAPI spec either has no
responsesat all, or produces{"not":{}}schemas due to the numeric key corruption.After: Full response schemas are generated automatically:
{ "responses": { "200": { "content": { "application/json": { "schema": { "type": "object", "required": ["id", "name", "email"], "properties": { "id": { "type": "string" }, "name": { "type": "string" }, "email": { "type": "string" } } } } } } } }Test plan
exportsmaps)integratetest still fails due totscnot on PATH in CI — pre-existing issue)This code was written by Claude (Opus 4.6) and reviewed by @alexng353.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit