fix(core): generate row types for repeater fields - #2469
Conversation
`fieldTypeToTypeScript` had no `repeater` case, so every repeater field fell through to `default: return "unknown"` -- while the zod side derives a precise row schema from the same `validation.subFields` metadata. This was an oversight rather than a decision. The emitter's switch, including its deliberate `case "json": return "unknown"`, dates from 43fcb9a and predates the field type: emdash-cms#111 added `repeater` without touching this file at all, so neither the zod case nor the TS case arrived with the feature. emdash-cms#2458 then added the zod case and left the TS side as it was. Type generation is now the only consumer of `subFields` that discards it -- media-usage extraction and the admin field editor both read it. Consumers cannot index or iterate `unknown`, so they declare row shapes by hand and cast to them. Those casts silence the compiler: when a sub-field is renamed or retyped, the generated type stays `unknown`, the cast keeps asserting the old shape, and the drift surfaces as a silently blank render rather than a type error. - Build an inline row type from `subFields`, reusing the existing per-type mapping so a `select` sub-field enumerates its options and an `image` sub-field emits the same media literal as a top-level image. - Type a sub-field that is not required as `T | null`, since `generateRepeaterRowSchema` applies `.nullish()` to it. A bare `T` would trade an honest `unknown` for an unsound type. - Keep emitting `unknown` when `subFields` is absent or empty. A repeater with no declared rows is schema-valid, and `{}[]` would be falsely permissive. `fieldTypeToTypeScript` now takes a structural subset of `Field` so a sub-field can reuse the mapping without a type assertion. Sub-field slugs match `/^[a-z][a-z0-9_]*$/`, so they are valid bare identifiers and need no quoting in the emitted literal.
🦋 Changeset detectedLatest commit: dc61d74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
recheck |
github-actions
Bot
added
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
|
this test failing but my changes dont really touch any of this? 🤔 |
|
Our CI pipeline has been a bit too flaky recently 😬 |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right change: repeater fields were the only validation.subFields consumer that still discarded the metadata and fell through to unknown, leaving generated types out of sync with the runtime zod schema. The PR aligns the TypeScript emitter with generateRepeaterRowSchema, emits the inline row type with correct nullability, and keeps the empty/absent subFields fallback to unknown so the type stays honest. The minor bump and changeset are appropriate because narrowing unknown can break downstream casts.
I read the diff, the full zod-generator.ts/types.ts source, the typegen/schema export routes, the test file, and existing changesets. The implementation is functionally correct and matches the runtime schema shape (nullish optional sub-fields, option enumeration for selects, media literal for images, no index signature).
Two non-blocking AGENTS.md/quality notes:
- One of the new tests is a tautological config-pin that asserts the implementation's exact concatenated output back at itself.
- A couple of new comments justify design decisions by referencing the current implementation rather than stating an invariant. The codebase has similar existing comments, but these are easy to tighten.
No blocking issues.
| it("maps every allowed sub-field type", () => { | ||
| const ts = generateTypeScript( | ||
| makeRepeaterCollection([ | ||
| { slug: "a", label: "A", type: "string", required: true }, | ||
| { slug: "b", label: "B", type: "text", required: true }, | ||
| { slug: "c", label: "C", type: "url", required: true }, | ||
| { slug: "d", label: "D", type: "number", required: true }, | ||
| { slug: "e", label: "E", type: "integer", required: true }, | ||
| { slug: "f", label: "F", type: "boolean", required: true }, | ||
| { slug: "g", label: "G", type: "datetime", required: true }, | ||
| ]), | ||
| ); | ||
|
|
||
| expect(ts).toContain( | ||
| "specs?: { a: string; b: string; c: string; d: number; e: number; f: boolean; g: string }[];", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
[suggestion] This test is a tautological config-pin. It asserts the exact concatenated string that the implementation happens to produce, so it passes only because the test constant matches the current switch ordering and output formatting. The surrounding tests already verify the real behaviors (row object shape, optional-nullability, select enumeration, image literal, required fallback). If a new allowed sub-field type is added or the emitted spacing changes, this test will fail even though the mapping is correct.
Consider deleting it or replacing it with per-type assertions that don't pin the exact combined output.
| // `generateRepeaterRowSchema` applies `.nullish()` to a sub-field that | ||
| // is not required, so `null` is a legal stored value. |
There was a problem hiding this comment.
[suggestion] This comment justifies a design decision by referencing the current implementation of generateRepeaterRowSchema ("applies .nullish()"). Per AGENTS.md, comments should explain non-obvious invariants, not justify decisions or couple to another function's implementation. If generateRepeaterRowSchema later stops using .nullish(), this comment becomes stale.
If you keep a comment here, state the invariant for this emitter ("Optional sub-fields may be null at runtime") instead of describing how the zod schema achieves it.
| // A repeater carrying no sub-fields describes no rows, so `{}[]` would | ||
| // be falsely permissive. |
There was a problem hiding this comment.
[suggestion] This comment explains the unknown fallback, but the previous line already reads if (!subFields || subFields.length === 0) return "unknown";, which is clear enough. The justification for the design belongs in the PR description/changeset; the code's intent is already plain here.
Consider removing this comment or replacing it with a shorter invariant if you think future readers will otherwise expect {}[].
MA2153
left a comment
MA2153
left a comment
There was a problem hiding this comment.
Approach looks right: the missing case reads as a gap rather than a decision, the nullability matches generateRepeaterRowSchema, and delegating sub-fields back through the same mapper is the right call. Checked out the branch, tests pass.
Three things worth fixing first, all in the new repeater case, left inline. Two of them turn a previously inert unknown into a generated file that doesn't compile.
Smaller notes:
"maps every allowed sub-field type"doesn't actually coverselectorimage(both covered separately, fine) and won't fail when a new entry is added toREPEATER_SUB_FIELD_TYPES. Driving it from that const would make it a real guard. Aselectsub-field with nooptionsis untested.- The
`{}[]` would be falsely permissivecomment is justification for a decision, which the comment rules exclude. The.nullish()one is worth keeping, it's a cross-file invariant a reader would otherwise get wrong. - Pre-existing and out of scope, but the sub-field path inherits it:
selectoptions are interpolated into string literals unescaped, and unlike slugs they have no character constraint even on the API path.
| return subField.required | ||
| ? `${subField.slug}: ${type}` | ||
| : `${subField.slug}?: ${type} | null`; |
There was a problem hiding this comment.
Sub-field slugs go in as bare identifiers, but the /^[a-z][a-z0-9_]*$/ guarantee only holds on the admin API route (repeaterSubFieldSchema, api/schemas/schema.ts:59). Seed files bypass it: SeedField.validation is Record<string, unknown> (seed/types.ts:111), validateSeed checks pattern and uniqueness for top-level field slugs only, and apply.ts:224 hands validation straight to registry.createField, which stringifies it without looking. Rows written before that route schema existed are unchecked too.
Ran generateTypeScript on this branch with such a field:
specs?: { first-name: string }[]; // slug "first-name"
specs?: { a: any }[] | { evil: string }[]; // slug 'a: any }[] | { evil'
The first is TS1131, so emdash-env.d.ts stops parsing and every collection loses its types. On main both emit unknown. It also doesn't self-heal: the integration rewrites the file on each dev server start whenever content differs (astro/integration/index.ts:660), so fixing it by hand doesn't stick, and the committed file is what astro check reads in CI.
JSON.stringify(subField.slug) as the member name covers any string. Falling back to unknown when a slug fails the pattern works too.
| // be falsely permissive. | ||
| if (!subFields || subFields.length === 0) return "unknown"; | ||
|
|
||
| const members = subFields.map((subField) => { |
There was a problem hiding this comment.
Duplicate sub-field slugs give TS2300, and this one is reachable from the admin without a seed file. FieldEditor derives each sub-field slug from its label with no uniqueness check (FieldEditor.tsx:583), and subFields: z.array(repeaterSubFieldSchema).min(1) has no uniqueness refinement. Two sub-fields labelled "Name" both get name:
specs?: { name: string; name: number }[]; // TS2300 x2
generateRepeaterRowSchema tolerates this silently, since shape[subField.slug] = schema is last wins, so the emitter is the only thing that breaks. Deduping last wins here would match the zod side. Rejecting duplicates in the API schema is the better fix but a wider change.
| const subFields = field.validation?.subFields; | ||
| // A repeater carrying no sub-fields describes no rows, so `{}[]` would | ||
| // be falsely permissive. | ||
| if (!subFields || subFields.length === 0) return "unknown"; |
There was a problem hiding this comment.
Minor: this assumes an array, but on the seed/registry path validation is unvalidated JSON. subFields: {} gets past the length check and throws in the .map below. That degrades to a logged typegen warning rather than a broken file, so much milder than the other two, but the other consumer of this data guards it with Array.isArray (media/usage/content-fields.ts:117).
github-actions
Bot
added
review/awaiting-author
Quote member names, keep the last declaration of a duplicated slug, and fall back to `unknown` when `subFields` is not an array.
github-actions
Bot
added
review/needs-rereview
no prob! and thanks for reviewing, that array assumpption shouldnt have made it through, idk how i missed it, noticed itd also allow strings to pass through |
3 participants
What does this PR do?
Type generation emitted
unknownfor everyrepeaterfield, so consuming code could not index or iterate a repeater and had to declare row shapes by hand and cast to them.fieldTypeToTypeScripthad norepeatercase and fell through todefault: return "unknown", even though the zod side derives a precise row schema from the samevalidation.subFieldsmetadata. This PR adds the missing case, so a repeater emits an inline row type built from its sub-fields.Given a
productscollection with aspecsrepeater (namerequired,valueoptional) and agalleryrepeater (photorequired,captionoptional):Before
After
Why this was a gap rather than a decision
The neighbouring
case "json": return "unknown"is deliberate —jsonis untyped by design.repeaterwas only sharing that fallback by accident:43fcb9a1and predates the field type.repeaterwithout touchingzod-generator.tsat all, so neither the zod case nor the TS case arrived with the feature.generateRepeaterRowSchema) and left the TS side as it was.Type generation was the last consumer of
validation.subFieldsthat discarded it — media-usage extraction (content-fields.ts,projection-fingerprint.ts) and the adminFieldEditorall read it.Why the casts were the real cost
assilences the compiler. When a sub-field is renamed or retyped in the schema, the generated type staysunknown, the hand-written cast keeps asserting the old shape, andastro checkstill reports 0 errors. The component then reads a property that no longer exists, getsundefined, and renders empty markup instead of failing — so a rename becomes a silently blank section in production rather than a build error.Design decisions worth reviewing
T | null, becausegenerateRepeaterRowSchemaapplies.nullish()to it, sonullis a legal stored value. A bareTwould trade an honestunknownfor an unsound type.subFieldsabsent or empty still emitsunknown. A repeater with no declared rows is schema-valid (subFields: z.array(...).min(1).optional()), and{}[]would be falsely permissive..passthrough()at runtime. TS object types are not sealed for reads, and an index signature would make misspelled property access legal again — defeating the purpose of the change.fieldTypeToTypeScriptnow takes a structural subset ofField({ type; validation? }) so a sub-field can reuse the mapping without a type assertion. Sub-field types are a subset of field types, so aselectsub-field enumerates its options and animagesub-field emits the same media literal as a top-level image field, by delegation rather than duplication./^[a-z][a-z0-9_]*$/, so they are valid bare identifiers and need no quoting in the emitted literal.Why
minorand notpatchNo runtime behaviour changes, but this narrows
unknownto a concrete type, which revokesunknown's universal-assertion privilege.x as SomethingUnrelatedand assignment to a hand-written row type whose optional members are not nullable will now error. Consumers do not opt in either: the writer runs in theastro:server:setuphook and rewritesemdash-env.d.tswhenever content differs, so breakage lands on the next dev-server start or CIastro check.This follows the 0.23.0 precedent (#1349), where regenerating interface names was a minor with "users should regenerate and update any direct interface references". The 0.10.0 precedent that aligned generated
image/filetypes was a patch, but that change was purely additive — new optional properties could not break existing access.The changeset tells consumers to regenerate types and warns that casts which have drifted from the schema may now report errors.
Type of change
Checklist
pnpm typecheckpasses — one pre-existing error onmain, unrelated to this change; see notes belowpnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.Not applicable: no admin UI strings, and this is a bug fix rather than a feature.
AI-generated code disclosure
anthropic/claude-opus-5) via OpenCodeScreenshots / test output
Eight tests added to
packages/core/tests/unit/schema/zod-generator.test.ts, written before the fix. Six failed against unfixed source; the two asserting theunknownfallback passed, as they should:After the fix:
Full
packages/coresuite on Node 22: 455/459 files, 5677 tests passing.