From d5171c75bc59d5b19393adb08a0baa85bbce2265 Mon Sep 17 00:00:00 2001 From: Erny Sans Date: Sat, 22 Aug 2026 18:40:47 -0500 Subject: [PATCH] refactor: declare rejection messages on the schemas themselves The requiredKey wrapper existed to work around a type-inference collapse caused by strictNullChecks being off: zod carries a schema's optionality marker as a union with undefined, and with that flag off the union collapsed, so a required key whose schema was a union or a nullable was inferred as optional. The flag is now on and the collapse is gone -- verified by inferring an empty object against a schema with a required union key and observing it is now correctly rejected, where it previously type-checked clean. Simply deleting the wrapper would have been a regression, which is the part worth recording. It carried the rejection message, and without it zod reports a failed union as `Invalid input` -- it cannot know which member the caller intended -- and reports a failed string inside a nullable as `expected string`, which is true of the inner schema but false of the field. Both send a caller looking for the wrong fix. Zod accepts the message on the schema itself, which keeps the wording, keeps the key inferred as required, and drops the z.custom indirection. Both call sites now use that form. Three tests are added asserting the exact messages, because message quality is the thing a future cleanup would silently discard. Each was mutation-tested by removing the error parameter it guards: two turn red for the union and one for the nullable body. requiredKey is retained and marked deprecated rather than removed. It is a published export, so dropping it would break any consumer that imported it, and the inventory test asserting the public surface still lists it. The emitted parse, safeParse and Interface signatures are unchanged. The Schema objects' own declared types change from ZodCustom to ZodUnion and ZodNullable, which is what those schemas now are; the inferred output is identical, confirmed by compiling a consumer against the built package under strict mode, including that a null response body is still accepted. --- lib/interface/schema.d.ts | 38 +++++++++++++++++++++++++--------- lib/interface/schema.js | 38 +++++++++++++++++++++++++--------- lib/model/Block.d.ts | 2 +- lib/model/Block.js | 18 +++++++--------- lib/model/EventData.d.ts | 2 +- lib/model/Idempotency.d.ts | 2 +- lib/model/Idempotency.js | 20 ++++++++---------- src/interface/schema.ts | 38 +++++++++++++++++++++++++--------- src/model/Block.ts | 17 ++++++--------- src/model/Idempotency.ts | 19 +++++++---------- test/model/Block.test.ts | 17 +++++++++++++++ test/model/Idempotency.test.ts | 8 +++++++ 12 files changed, 142 insertions(+), 77 deletions(-) diff --git a/lib/interface/schema.d.ts b/lib/interface/schema.d.ts index 7501a67..e01db86 100644 --- a/lib/interface/schema.d.ts +++ b/lib/interface/schema.d.ts @@ -307,16 +307,34 @@ export declare const parseOrThrow: (schema: TSchema, * **`strictNullChecks` is now enabled, so the collapse no longer occurs.** * Verified by inferring `z.object({u: z.union([...])})` and observing that the * required key is now correctly reported as missing (`TS2741`) where it - * previously type-checked clean. The wrapper is therefore no longer necessary - * for its original purpose, and is retained only so that removing it — which - * changes the errors reported at its two call sites — is a deliberate change - * with its own tests rather than a side effect of a compiler-flag change. - * - * The trade-off it carries is error granularity: a failure reports one issue at - * the key's path rather than the inner schema's per-member detail, which is why - * the message is a required argument rather than a generic default. That - * trade-off is now a cost without a corresponding benefit, so prefer the inner - * schema directly for new code, and see the note above before adding a call. + * previously type-checked clean. Both former call sites have been migrated and + * their keys remain inferred as required without the wrapper. + * + * ## Use zod's native `error` parameter instead + * + * The replacement is not "delete the wrapper" — doing only that degrades the + * reported message. A failed `z.union` reports `Invalid input`, because zod + * cannot know which member the caller intended, and a failed `z.string()` + * inside a `.nullable()` reports `expected string`, which is true of the inner + * schema but false of the field. Both send a caller looking for the wrong fix. + * + * Declare the message on the schema itself, which preserves the wording *and* + * the correct inference, and drops the `z.custom` indirection: + * + * ```ts + * // instead of requiredKey(z.union([...]), 'Expected …') + * z.union([...], {error: 'Expected …'}) + * // instead of requiredKey(z.string().nullable(), 'Expected … or null') + * z.string({error: 'Expected … or null'}).nullable() + * ``` + * + * This is **retained rather than removed** because it is a published export and + * dropping it would be a breaking change for any consumer that imported it. + * + * @deprecated Declare the message on the schema with zod's native `error` + * parameter — `z.union([...], {error})` or `z.string({error}).nullable()` — + * which reports the same message, infers the key as required, and avoids + * wrapping the schema in `z.custom`. * * @template TSchema The inner schema, which performs the actual validation. * @param {TSchema} schema - Schema describing the accepted values. diff --git a/lib/interface/schema.js b/lib/interface/schema.js index ca41f88..df6fc7a 100644 --- a/lib/interface/schema.js +++ b/lib/interface/schema.js @@ -138,16 +138,34 @@ export const parseOrThrow = (schema, value, label) => { * **`strictNullChecks` is now enabled, so the collapse no longer occurs.** * Verified by inferring `z.object({u: z.union([...])})` and observing that the * required key is now correctly reported as missing (`TS2741`) where it - * previously type-checked clean. The wrapper is therefore no longer necessary - * for its original purpose, and is retained only so that removing it — which - * changes the errors reported at its two call sites — is a deliberate change - * with its own tests rather than a side effect of a compiler-flag change. - * - * The trade-off it carries is error granularity: a failure reports one issue at - * the key's path rather than the inner schema's per-member detail, which is why - * the message is a required argument rather than a generic default. That - * trade-off is now a cost without a corresponding benefit, so prefer the inner - * schema directly for new code, and see the note above before adding a call. + * previously type-checked clean. Both former call sites have been migrated and + * their keys remain inferred as required without the wrapper. + * + * ## Use zod's native `error` parameter instead + * + * The replacement is not "delete the wrapper" — doing only that degrades the + * reported message. A failed `z.union` reports `Invalid input`, because zod + * cannot know which member the caller intended, and a failed `z.string()` + * inside a `.nullable()` reports `expected string`, which is true of the inner + * schema but false of the field. Both send a caller looking for the wrong fix. + * + * Declare the message on the schema itself, which preserves the wording *and* + * the correct inference, and drops the `z.custom` indirection: + * + * ```ts + * // instead of requiredKey(z.union([...]), 'Expected …') + * z.union([...], {error: 'Expected …'}) + * // instead of requiredKey(z.string().nullable(), 'Expected … or null') + * z.string({error: 'Expected … or null'}).nullable() + * ``` + * + * This is **retained rather than removed** because it is a published export and + * dropping it would be a breaking change for any consumer that imported it. + * + * @deprecated Declare the message on the schema with zod's native `error` + * parameter — `z.union([...], {error})` or `z.string({error}).nullable()` — + * which reports the same message, infers the key as required, and avoids + * wrapping the schema in `z.custom`. * * @template TSchema The inner schema, which performs the actual validation. * @param {TSchema} schema - Schema describing the accepted values. diff --git a/lib/model/Block.d.ts b/lib/model/Block.d.ts index 79a6d7b..ab85e81 100644 --- a/lib/model/Block.d.ts +++ b/lib/model/Block.d.ts @@ -85,7 +85,7 @@ export declare namespace Block { */ const Schema: z.ZodObject<{ type: z.ZodEnum; - value: z.ZodCustom | unknown[], string | number | Record | unknown[]>; + value: z.ZodUnion, z.ZodArray]>; label: z.ZodString; width: z.ZodOptional; height: z.ZodOptional; diff --git a/lib/model/Block.js b/lib/model/Block.js index 81dea91..258e579 100644 --- a/lib/model/Block.js +++ b/lib/model/Block.js @@ -3,7 +3,7 @@ * Copyright Furcata. All Rights Reserved. */ import { z } from "zod"; -import { counter, parseOrThrow, parseResult, requiredKey, } from "../interface/schema.js"; +import { counter, parseOrThrow, parseResult, } from "../interface/schema.js"; /** * Namespace for content-block primitives used to compose rich-media sections * within {@link EventData.Interface} and other structured content documents. @@ -42,7 +42,7 @@ export var Block; * schema so it is the single source of truth for both the validation and the * inferred output type of the `value` field below. */ - const blockValueSchema = z.union([z.string(), z.number(), z.record(z.string(), z.unknown()), z.array(z.unknown())]); + const blockValueSchema = z.union([z.string(), z.number(), z.record(z.string(), z.unknown()), z.array(z.unknown())], { error: 'Expected a string, number, object or array block value' }); /** * Runtime schema producing {@link Interface}. * @@ -67,16 +67,12 @@ export var Block; * {@link Interface.type}, and that correspondence is the renderer's to * enforce rather than this schema's. * - * Wrapped in `requiredKey` because this key is required and its schema is a - * `z.union`, which zod inferred as an optional key under the - * `strictNullChecks: false` setting this package previously used. Without - * the wrapper {@link parse} would have returned a type claiming `value` may - * be absent when at runtime it never is. `strictNullChecks` is now enabled - * and the inference is correct without the wrapper, which is retained here - * only so that removing it is a deliberate change with its own tests rather - * than a side effect of a compiler-flag change; see `requiredKey`. + * Carries an explicit `error` message on the union itself, because zod + * reports a failed union as the unhelpful `Invalid input` — it cannot know + * which member the caller intended. Naming the accepted shapes is the + * difference between a caller seeing what to send and seeing nothing. */ - value: requiredKey(blockValueSchema, 'Expected a string, number, object or array block value'), + value: blockValueSchema, /** * See {@link Interface.label}. Required but permitted to be empty, because * a block with no caption is a legitimate authoring choice. diff --git a/lib/model/EventData.d.ts b/lib/model/EventData.d.ts index 5840bcc..43aa1f0 100644 --- a/lib/model/EventData.d.ts +++ b/lib/model/EventData.d.ts @@ -200,7 +200,7 @@ export declare namespace EventData { uid: z.ZodOptional>; blocks: z.ZodOptional; - value: z.ZodCustom | unknown[], string | number | Record | unknown[]>; + value: z.ZodUnion, z.ZodArray]>; label: z.ZodString; width: z.ZodOptional; height: z.ZodOptional; diff --git a/lib/model/Idempotency.d.ts b/lib/model/Idempotency.d.ts index b097219..3812fa4 100644 --- a/lib/model/Idempotency.d.ts +++ b/lib/model/Idempotency.d.ts @@ -154,7 +154,7 @@ export declare namespace Idempotency { progress: z.ZodOptional>; response: z.ZodOptional; + body: z.ZodNullable; truncated: z.ZodBoolean; }, z.core.$loose>>; lockExpires: z.ZodOptional>>; diff --git a/lib/model/Idempotency.js b/lib/model/Idempotency.js index 8e98b0a..5f2348a 100644 --- a/lib/model/Idempotency.js +++ b/lib/model/Idempotency.js @@ -4,7 +4,7 @@ */ import { z } from 'zod'; import { baseFirestoreShape } from '../interface/base_db.js'; -import { counter, nonEmptyString, parseOrThrow, parseResult, requiredKey, timestampLike, token, } from '../interface/schema.js'; +import { counter, nonEmptyString, parseOrThrow, parseResult, timestampLike, token, } from '../interface/schema.js'; /** * Namespace for idempotency records: the durable claim that makes a * client-initiated request execute at most once. @@ -52,14 +52,9 @@ export var Idempotency; /** * Schema for {@link Response}. * - * {@link Response.body} is required **and** nullable. It is wrapped in - * `requiredKey` because zod inferred a required `z.nullable` key as optional - * under the `strictNullChecks: false` setting this package previously used. - * That setting is now enabled and the inference is correct without the - * wrapper, which is retained pending a deliberate removal; see that helper. - * `null` here means the original response genuinely had no body, which is a - * different claim from the field being absent, so the distinction has to - * survive. + * {@link Response.body} is required **and** nullable. `null` here means the + * original response genuinely had no body, which is a different claim from + * the field being absent, so the distinction has to survive. */ const responseSchema = z.looseObject({ /** @@ -68,9 +63,12 @@ export var Idempotency; */ status: z.int().min(100).max(599), /** - * See {@link Response.body}. + * See {@link Response.body}. The `error` message is declared on the schema + * so a rejection names `null` as accepted; zod's default would report + * `expected string`, which is true of the inner schema but false of the + * field, and would send a caller looking for the wrong fix. */ - body: requiredKey(z.string().nullable(), 'Expected a response body string or null'), + body: z.string({ error: 'Expected a response body string or null' }).nullable(), /** * See {@link Response.truncated}. */ diff --git a/src/interface/schema.ts b/src/interface/schema.ts index 6e7da81..d3b9472 100644 --- a/src/interface/schema.ts +++ b/src/interface/schema.ts @@ -374,16 +374,34 @@ export const parseOrThrow = ( * **`strictNullChecks` is now enabled, so the collapse no longer occurs.** * Verified by inferring `z.object({u: z.union([...])})` and observing that the * required key is now correctly reported as missing (`TS2741`) where it - * previously type-checked clean. The wrapper is therefore no longer necessary - * for its original purpose, and is retained only so that removing it — which - * changes the errors reported at its two call sites — is a deliberate change - * with its own tests rather than a side effect of a compiler-flag change. - * - * The trade-off it carries is error granularity: a failure reports one issue at - * the key's path rather than the inner schema's per-member detail, which is why - * the message is a required argument rather than a generic default. That - * trade-off is now a cost without a corresponding benefit, so prefer the inner - * schema directly for new code, and see the note above before adding a call. + * previously type-checked clean. Both former call sites have been migrated and + * their keys remain inferred as required without the wrapper. + * + * ## Use zod's native `error` parameter instead + * + * The replacement is not "delete the wrapper" — doing only that degrades the + * reported message. A failed `z.union` reports `Invalid input`, because zod + * cannot know which member the caller intended, and a failed `z.string()` + * inside a `.nullable()` reports `expected string`, which is true of the inner + * schema but false of the field. Both send a caller looking for the wrong fix. + * + * Declare the message on the schema itself, which preserves the wording *and* + * the correct inference, and drops the `z.custom` indirection: + * + * ```ts + * // instead of requiredKey(z.union([...]), 'Expected …') + * z.union([...], {error: 'Expected …'}) + * // instead of requiredKey(z.string().nullable(), 'Expected … or null') + * z.string({error: 'Expected … or null'}).nullable() + * ``` + * + * This is **retained rather than removed** because it is a published export and + * dropping it would be a breaking change for any consumer that imported it. + * + * @deprecated Declare the message on the schema with zod's native `error` + * parameter — `z.union([...], {error})` or `z.string({error}).nullable()` — + * which reports the same message, infers the key as required, and avoids + * wrapping the schema in `z.custom`. * * @template TSchema The inner schema, which performs the actual validation. * @param {TSchema} schema - Schema describing the accepted values. diff --git a/src/model/Block.ts b/src/model/Block.ts index 89af95b..3b32a02 100644 --- a/src/model/Block.ts +++ b/src/model/Block.ts @@ -11,7 +11,6 @@ import { ParseResult, parseOrThrow, parseResult, - requiredKey, } from "../interface/schema.js"; /** @@ -87,7 +86,7 @@ export namespace Block { * schema so it is the single source of truth for both the validation and the * inferred output type of the `value` field below. */ - const blockValueSchema = z.union([z.string(), z.number(), z.record(z.string(), z.unknown()), z.array(z.unknown())]); + const blockValueSchema = z.union([z.string(), z.number(), z.record(z.string(), z.unknown()), z.array(z.unknown())], {error: 'Expected a string, number, object or array block value'}); /** * Runtime schema producing {@link Interface}. @@ -113,16 +112,12 @@ export namespace Block { * {@link Interface.type}, and that correspondence is the renderer's to * enforce rather than this schema's. * - * Wrapped in `requiredKey` because this key is required and its schema is a - * `z.union`, which zod inferred as an optional key under the - * `strictNullChecks: false` setting this package previously used. Without - * the wrapper {@link parse} would have returned a type claiming `value` may - * be absent when at runtime it never is. `strictNullChecks` is now enabled - * and the inference is correct without the wrapper, which is retained here - * only so that removing it is a deliberate change with its own tests rather - * than a side effect of a compiler-flag change; see `requiredKey`. + * Carries an explicit `error` message on the union itself, because zod + * reports a failed union as the unhelpful `Invalid input` — it cannot know + * which member the caller intended. Naming the accepted shapes is the + * difference between a caller seeing what to send and seeing nothing. */ - value: requiredKey(blockValueSchema, 'Expected a string, number, object or array block value'), + value: blockValueSchema, /** * See {@link Interface.label}. Required but permitted to be empty, because * a block with no caption is a legitimate authoring choice. diff --git a/src/model/Idempotency.ts b/src/model/Idempotency.ts index d8dc776..d55e295 100644 --- a/src/model/Idempotency.ts +++ b/src/model/Idempotency.ts @@ -11,7 +11,6 @@ import { ParseResult, parseOrThrow, parseResult, - requiredKey, timestampLike, TimestampLike, token, @@ -150,14 +149,9 @@ export namespace Idempotency { /** * Schema for {@link Response}. * - * {@link Response.body} is required **and** nullable. It is wrapped in - * `requiredKey` because zod inferred a required `z.nullable` key as optional - * under the `strictNullChecks: false` setting this package previously used. - * That setting is now enabled and the inference is correct without the - * wrapper, which is retained pending a deliberate removal; see that helper. - * `null` here means the original response genuinely had no body, which is a - * different claim from the field being absent, so the distinction has to - * survive. + * {@link Response.body} is required **and** nullable. `null` here means the + * original response genuinely had no body, which is a different claim from + * the field being absent, so the distinction has to survive. */ const responseSchema = z.looseObject({ /** @@ -166,9 +160,12 @@ export namespace Idempotency { */ status: z.int().min(100).max(599), /** - * See {@link Response.body}. + * See {@link Response.body}. The `error` message is declared on the schema + * so a rejection names `null` as accepted; zod's default would report + * `expected string`, which is true of the inner schema but false of the + * field, and would send a caller looking for the wrong fix. */ - body: requiredKey(z.string().nullable(), 'Expected a response body string or null'), + body: z.string({error: 'Expected a response body string or null'}).nullable(), /** * See {@link Response.truncated}. */ diff --git a/test/model/Block.test.ts b/test/model/Block.test.ts index ec53e25..ce98ff2 100644 --- a/test/model/Block.test.ts +++ b/test/model/Block.test.ts @@ -320,6 +320,23 @@ describe('Block.Schema', () => { }); }); + describe('rejection message for the value union', () => { + it('should name the accepted shapes rather than reporting a bare invalid input', () => { + const result = Block.safeParse({ ...validBlock(), value: true }); + expect(result.success).toBe(false); + const issue = result.issues?.find((entry) => entry.path === 'value'); + expect(issue?.message).toBe('Expected a string, number, object or array block value'); + }); + + it('should name the accepted shapes when the value is absent entirely', () => { + const { value: _omitted, ...withoutValue } = validBlock(); + const result = Block.safeParse(withoutValue); + expect(result.success).toBe(false); + const issue = result.issues?.find((entry) => entry.path === 'value'); + expect(issue?.message).toBe('Expected a string, number, object or array block value'); + }); + }); + describe('throwing form', () => { it('should throw a ParseError naming the shape', () => { expect(() => Block.parse({ type: Block.Type.text })).toThrow(ParseError); diff --git a/test/model/Idempotency.test.ts b/test/model/Idempotency.test.ts index 8b3b682..612e5cf 100644 --- a/test/model/Idempotency.test.ts +++ b/test/model/Idempotency.test.ts @@ -126,6 +126,14 @@ describe('Idempotency.Schema', () => { }); describe('the stored response', () => { + it('should name null as accepted when the body is rejected', () => { + const record = {...validRecord(), response: {status: 200, body: 42, truncated: false}}; + const result = Idempotency.safeParse(record); + expect(result.success).toBe(false); + const issue = result.issues?.find((entry) => entry.path === 'response.body'); + expect(issue?.message).toBe('Expected a response body string or null'); + }); + it('should accept a null body, which means the original response had none', () => { const record = {...validRecord(), response: {status: 204, body: null, truncated: false}}; const parsed = Idempotency.parse(record);