Skip to content

Give every FileDef subtype module a default export; name the field in thunk errors - #5802

Open
lukemelia wants to merge 2 commits into
mainfrom
cs-12169-filedef-subtype-modules-have-inconsistent-export-shapes-some
Open

Give every FileDef subtype module a default export; name the field in thunk errors#5802
lukemelia wants to merge 2 commits into
mainfrom
cs-12169-filedef-subtype-modules-have-inconsistent-export-shapes-some

Conversation

@lukemelia

@lukemelia lukemelia commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What this does

The FileDef subtype family had two export conventions: the shell-based modules export their def class both named and as the default, while 17 older leaf modules (the text, image, and audio families — markdown-file-def, png-image-def, mp3-audio-def, …) were named-only. A default import of a named-only module silently evaluates to undefined, and linksTo(TsFileDef) then fails far from the import at schema time with an error blaming a cyclic dependency — naming neither the field, the module, nor the actual cause. Authors had to probe Object.keys(ns) to find out which import form a given module wanted.

  • All 17 named-only subtype modules now also export default their primary class, matching the convention the rest of the family already follows, so both import forms work everywhere.
  • cardThunk receives the field name and owning prototype from every field initializer (contains/containsMany/linksTo/linksToMany); the parameter is required, so a future call site can't silently degrade the message. An undefined field card class throws an error naming the exact declaration — The card class for field 'src' on 'MyCard' was undefined — and describes both real causes: an import that doesn't match the module's export shape, and a genuine cycle needing the thunk form. (The offending module itself isn't knowable at that point — the import has already evaluated to undefined by the time the value reaches the field machinery.)
  • The thunk form gets the same guard: a thunk that resolves to undefined throws the same field-and-owner-named error at the first field.card read, instead of an anonymous TypeError: Cannot use 'in' operator… later in getFieldDefinitions. This matters because cause (2) of the message advises switching to the thunk form — that path now fails just as loudly and specifically.
  • FILEDEF_CODE_REF_BY_EXTENSION is exported from runtime-common (as Readonly — every consumer is a read) so the export-shape guard test can walk the registry and automatically cover subtypes added later.

Test plan

  • packages/host/tests/unit/filedef-export-shape-test.ts — walks every registered subtype extension plus the six family base modules a subtype author imports from (image-file-def, audio-file-def, video-file-def, font-file-def, three-d-model-def, file-api), loads each module, and asserts the named export exists, a default export exists, and (for single-class modules) that the two are the same class. What this pins is "a default import never yields undefined"; for the two modules carrying a second named-only class (image-file-def, zip-file-def) only the registered class is pinned to the default. 117 assertions, passing locally against a full dev stack; the local base realm also completed a from-scratch index over the modified modules.
  • packages/host/tests/unit/card-thunk-error-test.ts — drives the field decorator exactly as Babel's transform does through all four field types, asserting the error names the field, the owner, and both causes; covers the eager form (throws at decoration) and the thunk form (throws at first field.card read).
  • Typechecks pass across runtime-common and host; template-lint clean on the touched .gts files.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

    1 files      1 suites   1h 40m 47s ⏱️
2 836 tests 2 825 ✅ 11 💤 0 ❌
2 849 runs  2 838 ✅ 11 💤 0 ❌

Results for commit 296f100.

Realm Server Test Results

    1 files      1 suites   16m 6s ⏱️
2 174 tests 2 174 ✅ 0 💤 0 ❌
2 254 runs  2 254 ✅ 0 💤 0 ❌

Results for commit 296f100.

@lukemelia lukemelia left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Lens: I reviewed this for correctness of the new error path, completeness of the export-shape fix against the whole subtype registry, and whether adding a default export to card modules is safe given how the base realm indexes them.

Bottom line: this is a clean, well-motivated change with no blocking issues. The fix is complete, the mechanism is correct, and the tests pin what they claim. One non-blocking nit on a test comment (inline).

What lands right — and why it's more than cosmetic.

  • The motivation is real, not hypothetical. The boxel-cli skill docs already instruct card authors (and the assistant) to reach these leaf classes via a default import — packages/boxel-cli/plugin/skills/boxel-file-def/references/import-paths.md and .../boxel/references/quick-reference.md both show import PngDef from '…/png-image-def', import MarkdownDef from '…/markdown-file-def', etc. Against the named-only modules those imports evaluated to undefined and blew up far from the import with a cyclic-dependency red herring. This PR makes the documented usage actually work.
  • The fix is complete against the registry. I walked every module referenced by FILEDEF_CODE_REF_BY_EXTENSION (33 distinct modules) and each now has exactly one export default. The 17 touched here plus the shell/other modules (pdf, docx, pptx, xlsx, html, midi, video, 3d, zip, fonts) all export both forms, so the new guard test passes rather than tripping on an untouched module.
  • No double-indexing risk. Adding export default X where X is already a named export isn't a new pattern — the shell modules (pdf-file-def, docx-file-def, image-file-def, …) have shipped that exact shape and index cleanly, so the 17 leaf modules adopting it carry no new indexing behavior.
  • The error-path change is correct. All four field initializers (contains/containsMany/linksTo/linksToMany) now thread { fieldName, ownerPrototype } into cardThunk; the four remaining cardThunk() call sites are stored-thunk invocations, not creations, so nothing is missed. ownerPrototype.constructor?.name resolves to the declaring card's name (Broken in the test, via Broken.prototype.constructor.name). The card-thunk-error-test.ts drives the field decorator exactly as Babel does and asserts each of the four types names the field and owner — it would fail if any initializer dropped the context.

Assertion count checks out: 33 unique modules × (named + default + strictEqual) = 99, matching the test plan.

One non-blocking recommendation (see inline thread on filedef-export-shape-test.ts): the multi-leaf branch is currently unreachable and its comment cites a module that doesn't exist. Reword the comment; the defensive branch itself is fine as future-proofing.

Non-blocking, and the COMMENT event here is deliberate — approval is the human's call.

Comment thread packages/host/tests/unit/filedef-export-shape-test.ts Outdated
@lukemelia
lukemelia marked this pull request as ready for review August 18, 2026 02:47
@lukemelia
lukemelia requested review from a team and habdelra August 18, 2026 02:47

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] I reviewed this through one lens above all: adding a second export of a class is not cosmetic in this codebase, because a class's persisted adoptsFrom comes from a reverse map keyed on export name and populated first-key-wins. So the first question was whether 17 new default exports can move any class's code ref, and the second was whether the new error actually covers the failure modes it enumerates.

Bottom line: no blocking issues. The default-export additions are safe, for a reason that depends on statement order and is worth recording; the error-message change is a real improvement that covers one of the two spellings it advises. Two gaps worth closing — the thunk form still fails un-named, and the new guard test's coverage stops short of the modules a default import is most likely to be written against.

What lands right

  • The convention choice is the conservative one. export default X; trailing the named export class X means the AMD transpiler defines the named key first and Loader.setIdentities registers the named ref, so identifyCard output — and therefore meta.adoptsFrom on existing instances and the field refs in indexed definitions — is unchanged. Verified by reading defineLocalGetter (enumerable: true), the exportStatements ordering in amd-transpile/index.ts, and the !this.identities.has(...) guard in loader.ts. The 16 modules that already shipped this shape are the empirical confirmation. Detail and the ordering hazard are in the comment on packages/base/markdown-file-def.gts.
  • Driving the guard test off the extension registry rather than a hardcoded list is right: new subtypes are covered the day they're registered. I confirmed all 33 registered modules resolve to exactly one class name and all 33 have default exports, which is where the 99 assertions come from.
  • The error test calls the decorator the way Babel's transform does, so it pins the real setupFieldcardThunk path rather than a hand-built descriptor.

Recommendations, most to least valuable

  1. Guard the thunk branch too. linksTo(() => X) with X === undefined still yields field.card === undefined and dies later in getFieldDefinitions as TypeError: Cannot use 'in' operator to search for 'primitive' in undefined — no field name. Cause (2) of the new message tells authors to switch to exactly that form. fieldContext is already in the closure. See the comment on cardThunk's return in packages/base/card-api.gts.
  2. Extend the export-shape walk to the family's base modules (image-file-def, audio-file-def, video-file-def, font-file-def, three-d-model-def, file-api) — none is in the registry, so none is checked, and svg-image-def.gts already default-imports image-file-def today. See the comment on the walk in filedef-export-shape-test.ts.
  3. Say in that same comment that the guarantee is "a default import never yields undefined", not "a default import yields the class you named". image-file-def (ImageDef default, RasterImageDef named-only) and zip-file-def (ZipDef default, ArchiveEntryField named-only) are the two modules where a default import silently produces a valid wrong class that the new error can never catch — and they're the real examples that belong where gltf-model-def currently sits.
  4. Make fieldContext required: all four call sites pass it and cardThunk isn't exported, so the 'a field' fallback is unreachable. While there, constructor?.name ?? 'unknown card'||, since an anonymous class's .name is '', not nullish.
  5. Assert the export-shape guidance in card-thunk-error-test.ts, not just the field naming — cause (1) is the sentence this change exists for and nothing pins it.
  6. Readonly<Record<string, ResolvedCodeRef>> on the newly-public registry.

On the open thread

The thread about the gltf-model-def example in filedef-export-shape-test.ts is still open and no commit has landed since it was raised; recommendation 3 above is the concrete replacement text it was asking for. Nothing else in the discussion is settled or relitigated here.

On the "unstable" mergeability state

Not a code signal, as far as I can tell. The first full run on 296f100f was 20/20 host shards green plus realm-server green. A later run on the same commit shows shards 1, 4, 11 and 20 failed and 16 and 19 cancelled after ~6 hours — but the failing shards uploaded no test output at all (No files were found with the provided path: junit/host-testem.log), i.e. they died before testem produced results, and the merged "Host Test Results" check for the same commit still reports 2 836 tests / 0 failed. That reads as harness/boot failure in the re-run rather than anything in this diff. Worth a re-run to get the status clean before merge rather than a code change.

Adjacent, out of scope

  • '.mismatch' is a test-only entry sitting in the production FILEDEF_CODE_REF_BY_EXTENSION (module ./filedef-mismatch, which doesn't exist), which is why the new test needs a skip at all. Pre-existing; whoever does the TODO: Replace with realm metadata configuration work is the natural person to move it into the fixture that needs it.
  • RasterImageDef living in image-file-def.gts as a named-only second class is the structural reason recommendation 3 exists. Splitting it into its own module would make the family's "one def class per module, exported both ways" rule total instead of near-total. Not this PR.

Generated by Claude Code

Comment thread packages/base/card-api.gts Outdated
Comment thread packages/base/card-api.gts Outdated
Comment thread packages/host/tests/unit/filedef-export-shape-test.ts
Comment thread packages/host/tests/unit/card-thunk-error-test.ts
Comment thread packages/base/markdown-file-def.gts
Comment thread packages/runtime-common/file-def-code-ref.ts Outdated
lukemelia and others added 2 commits August 18, 2026 16:57
… thunk errors

The subtype family had two export conventions: the shell-based modules
export their def class both named and default, while 17 older leaf modules
(text/image/audio families) were named-only. A default import of a
named-only module evaluates to undefined, which then fails far from the
import at schema time with an error suggesting a cyclic dependency —
naming neither the field nor the cause. All subtype modules now export
their primary class both ways.

cardThunk now receives the field name and owning prototype from every
field initializer (contains/containsMany/linksTo/linksToMany), so an
undefined field card class throws an error naming the exact field and card
and describing both real causes: an import that doesn't match the module's
export shape, and a genuine cycle needing the thunk form.

FILEDEF_CODE_REF_BY_EXTENSION is exported so the new export-shape test can
walk the registry and stay current as subtypes are added.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- cardThunk now also guards the thunk form: a thunk resolving to
  undefined throws the same field-and-owner-named error at first
  field.card read, instead of an anonymous TypeError later in
  getFieldDefinitions. fieldContext is required (all call sites pass
  it), and the owner name falls back via || so an anonymous class's
  empty-string name doesn't slip through.
- FILEDEF_CODE_REF_BY_EXTENSION is Readonly; every use is a read.
- The export-shape walk skips synthetic entries by shape (bare
  relative specifier) rather than by literal key, covers the six
  family base modules a subtype author imports from, and its comments
  now cite the real multi-class modules (image-file-def, zip-file-def)
  and state the guarantee precisely: a default import never yields
  undefined, not "yields the class you named".
- card-thunk-error-test asserts the guidance text (export shape +
  cyclic dependency) alongside the field naming, and adds the
  thunk-form cases through all four field types.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lukemelia
lukemelia force-pushed the cs-12169-filedef-subtype-modules-have-inconsistent-export-shapes-some branch from 741f8c2 to fb7a888 Compare August 18, 2026 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants