-
-
Notifications
You must be signed in to change notification settings - Fork 415
🔧 fix: use Static<T> for parse/safeParse return types #1598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🔧 fix: use Static<T> for parse/safeParse return types #1598
Conversation
WalkthroughTightened public TypeScript types: parse/safeParse now return runtime value types via Static, ModelValidator generic constrained to T extends TSchema, and related validator interfaces adjusted; unit tests added for schema validation and type inference. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)src/schema.ts (2)
src/types.ts (2)
🔇 Additional comments (6)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/schema.ts (1)
981-1006: safeParse catch blocks reference uninitializedcompiledvariable, causing ReferenceErrorIn three branches where local
validatorobjects are constructed and returned early (before line 1279 wherecompiledis assigned), thesafeParsecatch blocks referencecompiled.Errors(v). Sincecompiledremains uninitialized in these branches, accessing it triggers a temporal dead zone error.The corresponding
parsemethods correctly usevalidator.Errors(v), confirming the fix. Replacecompiled.Errors(v)withvalidator.Errors(v)in:
- Line 988 (standard + extra validators branch)
- Line 1158 (dynamic TypeBox branch)
- Line 1229 (dynamic standard-schema branch)
🧹 Nitpick comments (1)
test/validator/schema-validator.test.ts (1)
62-165: safeParse tests correctly pin the new result shape and type inferenceThe safeParse tests validate:
- success branch:
success: true, populateddata,error === null.- failure branch:
success: false,data === null,erroranderrorspresent.- Static-style inference via explicit
number/stringannotations onresult.datafields.One optional improvement: if you want to assert union inference more strongly, you could add a compile-time-only check like assigning
messageResult.datato a variable typed as the union’s static type (e.g.typeof eventPayload.static) so the compiler will fail if it ever regresses.If you want, I can sketch a small extra union-inference test that relies purely on TypeScript’s type checking without affecting runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/index.ts(1 hunks)src/schema.ts(2 hunks)src/types.ts(2 hunks)test/validator/schema-validator.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/validator/schema-validator.test.ts (1)
src/schema.ts (1)
getSchemaValidator(742-1399)
src/schema.ts (1)
src/index.ts (1)
Static(8245-8245)
src/types.ts (2)
src/index.ts (3)
ModelValidator(8220-8220)TSchema(8245-8245)Static(8245-8245)src/type-system/index.ts (1)
TypeCheck(682-682)
🔇 Additional comments (5)
test/validator/schema-validator.test.ts (1)
6-60: Parse tests exercise key behaviors and match current validator semanticsThe parse suite covers happy-path, additionalProperties toggling, and invalid data cases in a way that aligns with getSchemaValidator’s current behavior; this is a good regression net for the new Static-based typings as well.
src/index.ts (1)
453-462: models getter typing now correctly respectsModelValidator<T extends TSchema>Switching to
ModelValidator<Extract<Definitions['typebox'][K], TSchema>>is consistent with the newT extends TSchemaconstraint onModelValidatorand with howdefinitions.typeboxis populated at runtime (TypeBox-only). The change is type-level only and shouldn’t affect behavior; it just prevents accidentally threading non-TSchema entries into ModelValidator.If, in the future,
Definitions['typebox']can contain non-TSchema entries, consider an alias-style mapped type that drops those keys entirely instead of effectively giving themModelValidator<never>, but that’s not necessary with the current design.Please make sure
DefinitionBase['typebox']is indeed constrained to TypeBox schemas in your intended public surface so that this Extract remains sound.src/schema.ts (1)
2-11: ElysiaTypeCheck now correctly exposes Static-based value types from TypeBoxImporting
Staticand updatingElysiaTypeCheck<T extends TSchema>so that:
CleanreturnsStatic<T>,parsereturnsStatic<T>,- the safeParse success variant has
data: Static<T>,
brings the public typing in line with how validators are actually produced (Value.Decode,TypeCompiler.Compile, andcreateMirrorall operate on the static value shape, not the schema AST). This directly addresses the issue whereparse/safeParsepreviously surfacedTObject<...>-like schema types instead of{ id: number; name: string }-style runtime values, and it matches how the new tests useresult.data.idandresult.data.nameas primitives.Given that ElysiaTypeCheck extends
Omit<TypeCheck<T>, 'schema'>and defines these methods itself, the narrowing toStatic<T>here is sound with respect to TypeBox’s generics.Please run
tscagainst a few representative validators (object, union, transform) to confirm thatparse/safeParsenow resolve to the sameStatic<T>shape asTypeBox.Static<typeof schema>.Also applies to: 38-47
src/types.ts (2)
2-2: LGTM!The
Staticimport is correctly added to support the return type changes inModelValidator.
356-366: Core type fixes look correct.The changes properly address issue #1597:
Static<T>for return types ensuresparseandsafeParsereturn the resolved value type (e.g.,{ id: number; name: string }) instead of the schema type- The
success: falsefix in the failure branch corrects what was a bug (previously typed astrue)- The
T extends TSchemaconstraint properly restricts the genericOne observation: the parameter type
a: Ttakes the schema type, but semanticallyparse/safeParsereceive values to validate (should beunknown). TypeBox's ownTypeCheck.Decodeusesunknownfor input. This appears to be a pre-existing pattern, but worth verifying that it doesn't cause type inference issues in practice.
|
I decided to roll my own code because |
Fixes #1597
parse()andsafeParse()now return correct TypeScript typesStatic<T>instead ofTfor return types inElysiaTypeCheckandModelValidatorTextendsTSchemaconstraint to ModelValidatorsafeParsefailure case fromsuccess: truetosuccess: falseExtract<T, TSchema>for models getter type constraintSummary by CodeRabbit
Type System Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.