diff --git a/.changeset/nav-item-input-type.md b/.changeset/nav-item-input-type.md new file mode 100644 index 0000000000..da76454710 --- /dev/null +++ b/.changeset/nav-item-input-type.md @@ -0,0 +1,64 @@ +--- +"@objectstack/spec": minor +--- + +fix(spec): `defineApp` type-checked its navigation again — the #4171 fix only covered half the annotation (#3786) + +`NavigationItemSchema` is recursive, so it cannot infer its own type and carries a +hand-written annotation instead. #4171 fixed that annotation's **Output** half: it +had been `z.ZodType`, which made the exported `NavigationItem` `any` for every +consumer — a type that constrains nothing, which reads exactly like a type that +works. + +`z.ZodType` takes two parameters, ``, and **`Input` defaults to +`unknown`**. Naming only the first left the input half at that default, so +`z.input` resolved `navigation` to `unknown` — and `unknown` +accepts everything. `defineApp(config: z.input)` is the documented +authoring entry point, and it took + +```ts +defineApp({ + name: 'my_app', + label: 'My App', + navigation: [{ totally: 'made up' }, 42, 'nonsense'], // compiled clean +}); +``` + +with no complaint. Every authoring path through the app schema — `AppInput`, +`NavigationAreaSchema`, `NavigationContributionSchema` — was unchecked the same way. +Parsing was never affected: the schema rejected all of the above at runtime. It was +only the compile-time contract that lied, which is why nothing in the test suite +noticed. #4171's fix was verified through `z.infer`; nobody re-measured `z.input`, +and half a fix looks identical to a whole one. + +**The fix.** A new exported `NavigationItemInput` describes the authoring side, and +the annotation now names both parameters. The two unions genuinely differ and one +cannot serve both: `GroupNavItemSchema.expanded` and `UrlNavItemSchema.target` carry +`.default()`, so those keys are **required** in the parsed output and **omissible** +when authoring. Reusing `NavigationItem` as the input type would force authors to +write values the schema exists to supply. + +**What this changes for you.** Nothing at runtime, and nothing for code that reads a +parsed app. Code that *authors* navigation through `defineApp`, `AppInput`, or +`z.input` of any schema embedding `NavigationItemSchema` is now type-checked where it +previously was not, so genuinely malformed navigation that used to compile will now +surface as a compile error — the errors are pre-existing bugs becoming visible, not +new restrictions. Two notes on what the checked type says: + +- Authoring types (`AppInput`, `NavigationItemInput`) let you omit `expanded` and + `target`; the parsed types (`App`, `NavigationItem`) still guarantee both are + present. +- If you annotate a hand-written literal with the parsed type (`const APP: App = {…}`) + rather than the authoring type, you must spell out every defaulted key. That was + already true and is unchanged — this release just makes the authoring alternative + actually check its contents. + +**The mechanism, not a comment.** `src/ui/app.nav-type-assertions.ts` is a non-test +`src` module (the package's `tsc --noEmit` CI gate excludes test files) holding +compile-level probes for both unions and, critically, for the **wiring** between them +and the schema. A correct `NavigationItemInput` that no schema references would leave +every authoring path back at `unknown` — precisely the state being fixed — so the +load-bearing assertions go through `AppInput` and fail the moment the annotation loses +its second parameter. Each probe was mutation-tested: dropping the `Input` parameter, +removing the `expanded` default, and leaking `children` onto a flat nav branch each +turn the corresponding assertion red. diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 047ab404ec..6ff390e965 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -3345,6 +3345,7 @@ "NavigationContribution (type)", "NavigationContributionSchema (const)", "NavigationItem (type)", + "NavigationItemInput (type)", "NavigationItemSchema (const)", "NavigationModeSchema (const)", "Notification (type)", diff --git a/packages/spec/src/kernel/platform-capabilities.ts b/packages/spec/src/kernel/platform-capabilities.ts index 0e50ecbbc7..80e4f4cbce 100644 --- a/packages/spec/src/kernel/platform-capabilities.ts +++ b/packages/spec/src/kernel/platform-capabilities.ts @@ -180,13 +180,22 @@ export const PLATFORM_CAPABILITY_PROVIDERS: Readonly`, so the exported + * `NavigationItem` was `any`. A consumer importing it got a type that + * constrains nothing, which reads exactly like a type that works. + * 2. The `Input` half of that same annotation stayed at its `unknown` default + * even after #4171 typed the `Output` half — so `z.input` + * left `navigation` unchecked, and `defineApp` (the documented authoring + * entry point, `config: z.input`) accepted + * `navigation: [{ totally: 'made up' }, 42, 'nonsense']` with a clean + * compile. The #4171 fix was verified through `z.infer`; nobody re-measured + * `z.input`, and half a fix looks identical to a whole one. + * + * Both failures share the shape this file exists to break: a type that accepts + * too much reports nothing. A runtime test cannot catch either — the schema + * parses correctly in both cases; it is the compile-time contract that lies. + * + * Why a plain src module and not a test: `packages/spec/tsconfig.json` EXCLUDES + * every test file, and the CI gate for this package is `tsc --noEmit` over the + * `src` tree (lint.yml), so probes must live in a non-test src file to be + * checked at all. The file is referenced by no tsup entry and re-exported by no + * barrel, so it adds nothing to any build; it exists purely to make + * `tsc --noEmit` fail when the types and the schema drift apart again. + * + * The `@ts-expect-error` probes assert the NEGATIVE direction — if the type + * ever starts accepting those shapes, the now-unused suppression becomes the + * compile error. They are the half that catches "accepts too much", which is + * the half both real failures landed in. + */ + +import type { AppInput, NavigationItem, NavigationItemInput } from './app.zod'; + +/* ──────────────────────────────────────────────────────────────────────────── + * Output side (`z.infer`) — what a parse RETURNS. + * Defaulted keys are present, so they are required here. + * ──────────────────────────────────────────────────────────────────────────── */ + +const asItem = (x: NavigationItem): NavigationItem => x; + +/** A group holding children, including a group nested under a group. */ +export const groupWithChildren: NavigationItem = { + id: 'group_probe', + type: 'group', + label: 'Probe', + expanded: false, + children: [ + { id: 'child_page', type: 'page', label: 'Child', pageName: 'probe_page' }, + { id: 'nested_group', type: 'group', label: 'Nested', expanded: true, children: [] }, + ], +}; + +/** An object item nesting its views — `children` stays OPTIONAL on this branch. */ +export const objectWithChildren: NavigationItem = { + id: 'obj_probe', + type: 'object', + label: 'Probe', + objectName: 'probe_object', + children: [{ id: 'obj_view', type: 'object', label: 'View', objectName: 'probe_object' }], +}; + +/** The same branch without children — legal, unlike `group`. */ +export const objectWithoutChildren: NavigationItem = { + id: 'obj_bare', + type: 'object', + label: 'Bare', + objectName: 'probe_object', +}; + +/** + * `expanded` is REQUIRED on the output side: `GroupNavItemSchema` declares it + * `.default(false)`, so a parsed group always carries it. If this suppression + * goes unused the default was dropped from the schema — and every consumer + * reading `item.expanded` off a parsed app silently started getting + * `undefined`. + */ +// @ts-expect-error — parsed groups always carry `expanded` +asItem({ id: 'group_bare', type: 'group', label: 'Bare', children: [] }); + +/** Only the `object` and `group` branches nest. */ +// @ts-expect-error — `children` exists on no other branch +asItem({ id: 'page_probe', type: 'page', label: 'Probe', pageName: 'probe_page', children: [] }); + +/* ──────────────────────────────────────────────────────────────────────────── + * Input side (`z.input`) — what an AUTHOR writes. + * Defaulted keys are omissible; everything else is still checked. + * ──────────────────────────────────────────────────────────────────────────── */ + +const asInput = (x: NavigationItemInput): NavigationItemInput => x; + +/** + * The authoring shape: defaulted keys omitted. This is what cloud's admin app + * and every downstream `defineApp` caller writes. Under the `unknown` input + * this compiled for the uninteresting reason — everything did. + */ +export const inputOmitsDefaults: NavigationItemInput = { + id: 'group_probe', + type: 'group', + label: 'Probe', + children: [ + // `target` is `.default('_self')`, so it is omissible here and required above. + { id: 'link', type: 'url', label: 'Link', url: 'https://example.com' }, + ], +}; + +/** Supplying a defaulted key explicitly stays legal. */ +export const inputStatesDefaults: NavigationItemInput = { + id: 'group_probe2', + type: 'group', + label: 'Probe', + expanded: true, + children: [], +}; + +/** + * The load-bearing negatives. Each of these compiled clean while `Input` sat at + * `unknown` — that is the whole bug, so these are the assertions that would + * have caught it. + */ +// @ts-expect-error — a nav item is not an arbitrary record +asInput({ totally: 'made up', not: 'a nav item' }); + +// @ts-expect-error — a nav item is not a number +asInput(42); + +// @ts-expect-error — a nav item is not a string +asInput('nonsense'); + +// @ts-expect-error — `type` must be one of the nine declared discriminants +asInput({ id: 'bad_type', type: 'not_a_variant', label: 'X' }); + +// @ts-expect-error — `group` requires `children` on both sides +asInput({ id: 'group_bare', type: 'group', label: 'Bare' }); + +// @ts-expect-error — the members are `.strict()`; an undeclared key is not authoring +asInput({ id: 'grp', type: 'group', label: 'G', children: [], defaultOpen: true }); + +// @ts-expect-error — only `object` and `group` nest, on this side too +asInput({ id: 'page_probe', type: 'page', label: 'P', pageName: 'probe_page', children: [] }); + +/* ──────────────────────────────────────────────────────────────────────────── + * The halves must stay DIFFERENT. + * + * If someone "simplifies" by pointing both parameters at one union, the + * defaulted keys go wrong in one direction or the other: authors are forced to + * write `expanded`, or consumers stop being able to read it. These two pin the + * asymmetry itself. + * ──────────────────────────────────────────────────────────────────────────── */ + +/** Output is assignable to input — a parsed tree can be re-authored. */ +export const outputFeedsInput: NavigationItemInput = groupWithChildren; + +/** Input is NOT assignable to output: the defaults have not been applied yet. */ +// @ts-expect-error — `NavigationItemInput` is the looser half, by design +asItem(inputOmitsDefaults); + +/* ──────────────────────────────────────────────────────────────────────────── + * The WIRING — the assertions that actually catch the #4171 half-fix. + * + * Everything above pins the two unions. None of it pins the thing that broke: + * whether `NavigationItemSchema` is annotated with `NavigationItemInput` at + * all. A perfectly-written `NavigationItemInput` that no schema references + * leaves `z.input` at `unknown` and every authoring path + * unchecked — which is precisely the state this file was written to end. + * + * These go through `AppInput`, the type `defineApp` takes, so they fail the + * moment the annotation loses its second parameter (verified by mutation: + * dropping it back to `z.ZodType` turns all three into unused + * suppressions). + * ──────────────────────────────────────────────────────────────────────────── */ + +const asAppInput = (x: AppInput): AppInput => x; + +/** The shape every downstream author writes — defaulted keys omitted. */ +export const appInputOmitsDefaults: AppInput = { + name: 'probe_app', + label: 'Probe', + navigation: [{ id: 'grp', type: 'group', label: 'G', children: [] }], +}; + +// @ts-expect-error — nav entries are checked; `unknown` would swallow this +asAppInput({ name: 'probe_app', label: 'Probe', navigation: [42] }); + +// @ts-expect-error — …and this +asAppInput({ name: 'probe_app', label: 'Probe', navigation: [{ totally: 'made up' }] }); + +// NOTE: kept on one line — `@ts-expect-error` suppresses the NEXT LINE only, and +// the excess-property error lands on the nav entry, not on the opening `asAppInput(`. +// @ts-expect-error — …and this, the strictness the nav members declare +asAppInput({ name: 'p', label: 'P', navigation: [{ id: 'grp', type: 'group', label: 'G', children: [], defaultOpen: true }] }); diff --git a/packages/spec/src/ui/app.zod.ts b/packages/spec/src/ui/app.zod.ts index 3a56de4941..f36805d30d 100644 --- a/packages/spec/src/ui/app.zod.ts +++ b/packages/spec/src/ui/app.zod.ts @@ -442,17 +442,58 @@ export type NavigationItem = | SeparatorNavItem | GroupNavItem; +/** + * The INPUT half of {@link NavigationItemSchema} — what an author writes, as + * opposed to what a parse returns. + * + * `z.ZodType` takes TWO type parameters, ``, and **`Input` + * defaults to `unknown`**. #4171 fixed the annotation's Output half (it used to + * be `z.ZodType`, which made the exported `NavigationItem` `any` for every + * consumer) but left `Input` at that default — so `z.input` + * resolved `navigation` to `unknown`, and `unknown` accepts everything. The + * documented authoring entry point, `defineApp(config: z.input)`, + * therefore took `navigation: [{ totally: 'made up' }, 42, 'nonsense']` with a + * clean compile. That is the same "a type that constrains nothing" failure + * #4171 describes, surviving on the half nobody re-measured: the fix was + * verified through `z.infer` and `z.input` was never checked. + * + * The two halves genuinely differ, which is why one union cannot serve both: + * `GroupNavItemSchema.expanded` and `UrlNavItemSchema.target` carry + * `.default()`, so they are REQUIRED in the output and OPTIONAL here. Reusing + * {@link NavigationItem} as the input type would demand authors write values + * the schema exists to supply. + * + * Both recursive branches tie their `children` knot inline. The output union + * ties `object` inline but inherits `group`'s knot from the {@link GroupNavItem} + * alias — an asymmetry kept only because that alias is public API. + * `app.nav-type-assertions.ts` pins both unions at compile level. + */ +export type NavigationItemInput = + | (z.input & { children?: NavigationItemInput[] }) + | z.input + | z.input + | z.input + | z.input + | z.input + | z.input + | z.input + | (z.input & { children: NavigationItemInput[] }); + /** * Recursive Union of all navigation item types. * Allows constructing an unlimited-depth navigation tree. * - * The trailing cast to `z.ZodType` is forced by the member-array - * widening below: `discriminatedUnion` reports the output of a + * The trailing cast to `z.ZodType` is forced + * by the member-array widening below: `discriminatedUnion` reports the output of a * `ZodObject` member as `Record`, so the union's * inferred output carries no branch shapes at all. That fits the `z.ZodType` * this used to be annotated with — and nothing sharper, which is how the exported * `NavigationItem` came to be `any` for every consumer (#4171). * + * BOTH type parameters are spelled out. Naming only the first leaves `Input` at + * its `unknown` default, which silently un-checks every authoring path through + * this schema — see {@link NavigationItemInput}. + * * What the cast does NOT weaken: every branch of {@link NavigationItem} is * `z.infer`, so a key added to any variant's schema still * flows into the exported type with no edit here. What it leaves unchecked is @@ -460,7 +501,7 @@ export type NavigationItem = * type, or the reverse — which app.test.ts covers at runtime by parsing all nine * ("accepts every variant with its full declared payload"). */ -export const NavigationItemSchema: z.ZodType = z.lazy(() => +export const NavigationItemSchema: z.ZodType = z.lazy(() => // DISCRIMINATED on `type` (#4001 PR B). With `.strict()` members a plain // union would answer one unknown key with an `invalid_union` aggregate // listing all nine branches' failures; discriminating on `type` first means @@ -484,7 +525,7 @@ export const NavigationItemSchema: z.ZodType = z.lazy(() => // The members are lazySchema Proxies and a superRefine-wrapped variant, so // the array is widened for the discriminator-typed overload; runtime // discrimination works on all of them (asserted in app.test.ts). - ] as unknown as readonly [z.ZodObject, ...z.ZodObject[]]) as unknown as z.ZodType + ] as unknown as readonly [z.ZodObject, ...z.ZodObject[]]) as unknown as z.ZodType ); /**