feat: add MaybeNull validator #1585
Conversation
WalkthroughAdds a new MaybeNull type to the type system and public API ( 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)
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
🧹 Nitpick comments (1)
test/type-system/maybe-null.test.ts (1)
1-53: Expand test coverage for MaybeNull with additional types and edge cases.The current test suite only covers
MaybeNull(String())for OpenAPI schema generation. Consider adding tests for:
Other primitive types:
t.MaybeNull(t.Number())t.MaybeNull(t.Boolean())t.MaybeNull(t.Integer())Complex types:
t.MaybeNull(t.Object({ ... }))t.MaybeNull(t.Array(t.String()))Type validation:
- Verify that
MaybeNull(Number())rejects strings- Verify that
MaybeNull(String())rejects numbersBehavioral comparison:
- Document the difference in OpenAPI schema between
Nullable(union/anyOf) andMaybeNull(nullable flag)Example additional test:
it('MaybeNull works with various types', async () => { const app = new Elysia().post('/', ({ body }) => body, { body: t.Object({ count: t.MaybeNull(t.Number()), active: t.MaybeNull(t.Boolean()), tags: t.MaybeNull(t.Array(t.String())) }) }) const res = await app.handle(post('/', { count: null, active: true, tags: ['test'] })) expect(res.status).toBe(200) expect(await res.json()).toEqual({ count: null, active: true, tags: ['test'] }) }) it('MaybeNull rejects invalid types', async () => { const app = new Elysia().post('/', ({ body }) => body, { body: t.Object({ count: t.MaybeNull(t.Number()) }) }) const res = await app.handle(post('/', { count: 'not a number' })) expect(res.status).toBe(422) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/type-system/index.ts(3 hunks)test/type-system/maybe-null.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/type-system/maybe-null.test.ts (1)
src/type-system/index.ts (1)
t(683-683)
src/type-system/index.ts (1)
src/index.ts (2)
t(8126-8126)Static(8245-8245)
🔇 Additional comments (5)
src/type-system/index.ts (4)
1-1: LGTM: Import addition supports MaybeNull type signature.The
Staticimport is correctly added to support the type signature of the newMaybeNullfunction.
495-497: LGTM: Clear deprecation notice with migration path.The deprecation notice appropriately guides users to the new OpenAPI 3.0 compliant
MaybeNullfunction and clearly indicates the timeline for removal.
672-672: LGTM: Standard exposure pattern.Correctly exposes
MaybeNullon thetobject following the established pattern.
1-683: PR objectives regarding Number validation issue #1157 cannot be verified from the provided code snippet.The review comment claims the PR states it "Fixes issue #1157" related to Number validation producing union errors, but the code changes shown only add
MaybeNulland deprecateNullable. Without access to the PR description, git history, or the full changeset, it cannot be confirmed whether:
- The PR actually claims to fix #1157
- Number validation changes exist in other files not shown here
- The issue #1157 description matches what the review states
- These changes indirectly address the stated issue
To verify this concern, provide the PR description, the related issue details, or access to the full changeset across all modified files.
test/type-system/maybe-null.test.ts (1)
5-27: LGTM: OpenAPI schema generation tests are comprehensive.The tests correctly verify that
MaybeNullproduces OpenAPI 3.0 compliant schemas withnullable: truefor both primitive types and object properties.
|
This is nice, one little problem is this doesn't validate the value 
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/type-system/types.ts (1)
143-148: TMaybeNull shape looks good; consider tightening WrappedKind’s value type.Structurally this is fine and aligns with other tagged schemas. If you want stricter typing,
[WrappedKind]: TSchema[typeof Kind](or a dedicated alias) would more accurately reflect that you’re storing another schema’s kind rather than an arbitrary string, but it’s not required.src/type-system/index.ts (5)
25-29: Global error-function override: good for MaybeNull, but note the global side effect.Importing
Kind,Type, and the error helpers is appropriate here, and the error override below scopes special handling toMaybeNullwhile delegating others toDefaultErrorFunction. Just be aware this is a globalSetErrorFunctioncall; any user who also callsSetErrorFunctionwill either override or be overridden by this, so behavior will depend on import order.
83-96: MaybeNull runtime validation now correctly delegates to the wrapped schema and acceptsnull.The custom type handler reconstructs the original schema via
[Kind]: wrappedKind, ...schemaand checks it withValue.Check, then additionally allowsvalue === null. This addresses the earlier concern that MaybeNull only affected OpenAPI output without actually validating nulls. You might optionally short‑circuit fornullfirst to avoid an unnecessaryValue.Check, but it’s not required.-createType<TMaybeNull>( - 'MaybeNull', - ({ [Kind]: kind, [WrappedKind]: wrappedKind, ...schema }, value) => { - return ( - Value.Check( - { - [Kind]: wrappedKind, - ...schema - }, - value - ) || value === null - ) - } -) +createType<TMaybeNull>( + 'MaybeNull', + ({ [Kind]: kind, [WrappedKind]: wrappedKind, ...schema }, value) => { + if (value === null) return true + + return Value.Check( + { + [Kind]: wrappedKind, + ...schema + }, + value + ) + } +)
98-105: Custom error message for MaybeNull is clear; consider still honoring per-schema errors if needed.The
SetErrorFunctionoverride gives a nice, specific message for MaybeNull (Expected either ${error.schema.type} or null) and defers all other kinds toDefaultErrorFunction. If you intend to support schema‑level custom error messages on MaybeNull (e.g., viaerrorinSchemaOptions), you may want to check for and prioritizeerror.schema.errorhere before falling back to the generic string.
187-190: NumericEnum generic constraint is stricter and better models numeric enums.Constraining
NumericEnum<T extends AssertNumericEnum<T>>improves type safety by ensuring the enum‑like object matches the expected TS numeric-enum shape. This should remain source‑compatible for typicalenumand enum‑like objects, but it’s worth confirming that existing call sites (especially with plain object literals) still inferTwithout extra casts.
538-546: MaybeNull wrapper design aligns OpenAPI output with proper runtime validation; watch thedefaultoverride.The wrapper:
- Captures the underlying
[Kind]into[WrappedKind],- Keeps all other schema properties via
...schema,- Forces
default: undefinedandnullable: true,- Tags the schema as
[Kind]: 'MaybeNull'.This cleanly separates OpenAPI representation (
nullable: truewith the originaltype) from runtime behavior (handled by the customMaybeNulltype). One thing to double‑check: always overridingdefaulttoundefinedwill discard any default on the wrapped schema. If users expect a default value to survive through MaybeNull, you may want to only setdefaultwhen it is currentlyundefined, or document that MaybeNull intentionally clears the default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/type-system/index.ts(6 hunks)src/type-system/types.ts(1 hunks)test/type-system/maybe-null.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/type-system/maybe-null.test.ts
🔇 Additional comments (5)
src/type-system/index.ts (5)
5-24: Additional TypeBox type imports are consistent with later usage.
JavaScriptTypeBuilder,NumberOptions,StringOptions,TEnum, andUint8ArrayOptionsare all used below (Numeric, NumericEnum, string options, Uint8Array, andtaugmentation) and keep the typings aligned with TypeBox’s API.
39-52: New type imports from./typesare wired correctly.
AssertNumericEnum,ElysiaTransformDecodeBuilder,TArrayBuffer,TFile,TFiles,TMaybeNull, andWrappedKindare all referenced later and keep the public type surface coherent with the helpers in this module.
57-57: UsingValue.Checkfor MaybeNull’s inner validation is appropriate.Importing
Valuefrom@sinclair/typebox/valueis justified by the MaybeNull implementation; it reuses TypeBox’s runtime semantics for the wrapped schema instead of re-implementing checks.
529-531: Nullable deprecation notice clearly signals the preferred MaybeNull path.The deprecation JSDoc is explicit about OpenAPI 3.0 compliance and future removal, which should guide users to migrate gradually.
709-711: Public aliast.MaybeNullcorrectly forwards toElysiaType.MaybeNull.This keeps the API consistent with other helpers (
t.Nullable,t.MaybeEmpty, etc.) and exposes MaybeNull on the familiartbuilder surface.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review PR description |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3554 🏁 Script executed: Length of output: 2852 🏁 Script executed: Length of output: 7476 @akim-bow, I've reviewed the PR description and it's well-written overall! Here's my assessment: ✅ What's Good
💡 Suggested Improvements
The description is accurate and the PR correctly addresses the linked issue. The comprehensive test suite in |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/type-system/index.ts (2)
99-106: Improve error message generation for MaybeNull.The error message uses
error.schema.type, which may beundefinedfor complex schemas, resulting in messages like "Expected either undefined or null". Additionally, referencing the MaybeNull wrapper'styperather than the wrapped schema's semantics may be confusing.Consider using the
WrappedKindto generate more meaningful error messages:SetErrorFunction((error) => { switch (error.schema[Kind]) { case 'MaybeNull': - return `Expected either ${error.schema.type} or null` + const wrappedKind = error.schema[WrappedKind] || error.schema.type || 'value' + return `Expected ${wrappedKind.toLowerCase()} or null` default: return DefaultErrorFunction(error) } })This produces clearer messages like "Expected string or null" or "Expected number or null" instead of "Expected either string or null".
539-539: Consider returning TMaybeNull for better type safety.The function returns
TSchema, which loses the specificTMaybeNulltype information. While this maintains flexibility, returningTMaybeNullwould provide better type inference.-MaybeNull: <T extends TSchema>({ [Kind]: kind, ...schema }: T): TSchema => { +MaybeNull: <T extends TSchema>({ [Kind]: kind, ...schema }: T): TMaybeNull => {This change is optional and depends on whether downstream code relies on the generic
TSchematype.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/type-system/index.ts(6 hunks)test/type-system/maybe-null.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/type-system/maybe-null.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/type-system/index.ts (2)
src/type-system/utils.ts (1)
createType(29-36)src/type-system/types.ts (2)
TMaybeNull(145-148)WrappedKind(143-143)
🔇 Additional comments (6)
src/type-system/index.ts (6)
1-57: LGTM!The new imports (DefaultErrorFunction, SetErrorFunction, Value, TMaybeNull, WrappedKind) are all properly used in the MaybeNull implementation.
188-206: LGTM!Adding the type parameter
T extends AssertNumericEnum<T>improves type safety for NumericEnum by enforcing that the enum values are numeric, aligning with TypeBox conventions.
711-711: LGTM!The export follows the established pattern for Elysia type extensions.
539-547: Fix default value override in MaybeNull.Line 543 sets
default: undefined, which overrides any existing default from the input schema. For example,t.MaybeNull(t.String({ default: 'hello' }))will lose the 'hello' default.Apply this diff to preserve the original default when present:
MaybeNull: <T extends TSchema>({ [Kind]: kind, ...schema }: T): TSchema => { return { [Kind]: 'MaybeNull', [WrappedKind]: kind, - default: undefined, ...schema, nullable: true } as unknown as TMaybeNull },By removing
default: undefinedbefore spreading...schema, any existing default will be preserved. If no default exists, it will remain undefined naturally.Likely an incorrect or invalid review comment.
530-532: Clarify the OpenAPI version context in the deprecation notice.The deprecation comment states "Use MaybeNull instead which is OpenAPI 3.0 compliant", but doesn't explain the rationale. Confirm that this library prioritizes OpenAPI 3.0 compatibility (where
nullable: trueis the standard) over OpenAPI 3.1 alignment (which uses JSON Schema-style type unions). If compatibility with OpenAPI 3.0 is the design goal, make that explicit; otherwise, clarify what other requirement drives deprecatingNullablein favor ofMaybeNull.Also verify that the function's return type on line 539 is declared as
TSchemarather thanTMaybeNull, which would lose type information.
83-97: Verify thatValue.Checkfrom TypeBox properly handles custom types registered via TypeRegistry.The runtime validator reconstructs a schema with the wrapped kind and validates it using
Value.Check. However, TypeBox'sValue.Checkmay not automatically recognize custom types registered throughTypeRegistry(such asUnionEnum,ArrayBuffer,Files, etc.). WhenMaybeNullwraps these custom types, the validation could fail becauseValue.Checkdoesn't have access to the custom validator functions.Consider whether the validation should use an alternative approach that respects TypeRegistry entries. If
Value.Checkis confirmed to support custom types, add a test case witht.MaybeNull(t.UnionEnum([...]))or similar custom types to prevent regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/type-system/index.ts(6 hunks)test/type-system/maybe-null.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/type-system/maybe-null.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/type-system/index.ts (2)
src/type-system/utils.ts (1)
createType(29-36)src/type-system/types.ts (3)
TMaybeNull(145-148)WrappedKind(143-143)AssertNumericEnum(249-257)
🪛 GitHub Actions: Build and Test
src/type-system/index.ts
[error] 103-103: TypeScript error TS2538: Type 'unique symbol' cannot be used as an index type.
🔇 Additional comments (4)
src/type-system/index.ts (4)
1-58: LGTM!The new imports support the MaybeNull functionality correctly:
Valueenables runtime validation, and the error handling imports enable custom error messages.
84-98: LGTM!The runtime validation correctly accepts
nullvalues and validates non-null values against the wrapped schema. This addresses the concern from the previous review about runtime null validation.
531-548: LGTM!The deprecation notice is clear, and the
MaybeNullimplementation correctly preserves the original schema while settingnullable: truefor OpenAPI 3.0 compliance. The use ofWrappedKindto store the originalKindenables proper runtime validation.
712-712: LGTM!The export follows the established pattern for exposing type helpers on the
tAPI surface.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@SaltyAom yeap, i thought Typebox magically checks this and i messed up with tests.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/type-system/index.ts (3)
84-98: MaybeNull validator now correctly validates bothnulland the wrapped schema.The
MaybeNullTypeRegistry entry short-circuitsnulland delegates non-null values toValue.Checkon the underlying schema reconstructed from[WrappedKind], which fixes the earlier “schema-only, no runtime validation” issue and keeps behavior consistent with other TypeBox-driven checks. If this ever becomes a hot path, you might later consider caching/compiling the wrapped schema once instead of callingValue.Checkdirectly each time, but this is fine as-is.
100-107: Global TypeBox error handler override is correct but may surprise consumers.The custom branch for
Kind === 'MaybeNull'looks good and the cast toTMaybeNullwhen reading[WrappedKind]resolves the previous TS compile error. However,SetErrorFunctionis a global override for the entire TypeBox error pipeline; importing this module will change error formatting for any other TypeBox usage in the same process. If users are also customizing TypeBox errors elsewhere, whichever runs last wins. Please confirm this global override is intentional for Elysia, or consider scoping/customizing errors inside Elysia’s own error handling rather than at the library-global TypeBox level.
531-538: Nullable deprecation messaging is clear; consider mirroring ont.Nullablealias.The
@deprecatedJSDoc onElysiaType.Nullableclearly points users toMaybeNulland notes removal in the next major. Sincet.Nullableis also part of the public surface, you may want to ensure it surfaces the same deprecation hint in editor/IDE tooling (e.g., via d.ts comments or a mirrored JSDoc on the alias) so consumers see consistent guidance whichever entry point they use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/type-system/index.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/type-system/index.ts (2)
src/type-system/utils.ts (1)
createType(29-36)src/type-system/types.ts (3)
TMaybeNull(145-148)WrappedKind(143-143)AssertNumericEnum(249-257)
🔇 Additional comments (3)
src/type-system/index.ts (3)
189-207: NumericEnum helper is consistent and enforces strict numeric enum values.The
NumericEnumimplementation correctly constrainsTviaAssertNumericEnum<T>, compiles aType.Enumschema, and uses aTransformto accept both numeric strings (viaformat: 'numeric') and numbers while decoding strictly to a valid enum member (rejectingNaNand out-of-range values viacompiler.Check). This aligns well with the existingNumerichelper and looks sound.
540-548: MaybeNull builder wiring withWrappedKindandnullable: truelooks correct.
ElysiaType.MaybeNullpreserves the original schema (including anydefault), stores its original[Kind]into[WrappedKind], replaces[Kind]with'MaybeNull', and forcesnullable: true. Combined with thecreateType<TMaybeNull>('MaybeNull', ...)validator above, this gives OpenAPI 3.0–stylenullableoutput while still validatingnulland non-null values against the wrapped schema. The object spread order also correctly lets the originaldefaultoverride the interimdefault: undefined.
711-713: Publict.MaybeNullalias is correctly exposed alongsidet.Nullable.Wiring
t.MaybeNull = ElysiaType.MaybeNullmatches the pattern used for the other helpers and ensures the new API is available from the primarytnamespace without breaking existingt.Nullablecallers.
2 participants
Fixes #1577
Description
I've made additional validator to support usecase of generating proper OpenAPI schema if elysia.js is to use with external tools for generating clients or documentation. I also think that elysia should have a single method for creating nullable field instead of 2 almost equally validators. That's why i marked previous one as outdated to tell a user preferred validator to use. Probably in the future older one could be removed completely, i.e., in next major release.
If i miss some usecases why Nullable was implemented in a way it is i would be happy to correct my PR.
Summary by CodeRabbit
New Features
Deprecations
Tests
✏️ Tip: You can customize this high-level summary in your review settings.