fix(firestore)!: narrow composed Optional/OptionalNull json Encoded to reject null

[detail-app]

Detail bug report: View on Detail

Closes #100

Bug

Firestore.Optional(field) and Firestore.OptionalNull(field) (when wrapped around a multi-variant Field such as DateTime or Reference(id, path)) had a hand-written conditional return type whose json variant resolved to Schema.OptionFromOptionalNullOr — admitting null in typeof Model.json.Encoded. The runtime Model.fieldEvolve table that the helpers actually build the json variant with is Schema.OptionFromOptional, which rejects null with a SchemaError.

So a null value for such a field compiled against typeof Model.json.Encoded but threw at runtime — the type was wider than the decoder. The simple Optional(Schema) / OptionalNull(Schema) forms and OptionalDeletable were already correct; this only affected the composed Optional(Field) / OptionalNull(Field) branch of the conditional.

Fix

Type-only change in packages/effect-firebase/src/lib/firestore/model/optional.ts: carve json out of the non-database branch of both conditionals so it maps to Schema.OptionFromOptional<S[K]>, matching the runtime and mirroring OptionalDeletable's existing conditional. The runtime fieldEvolve tables are byte-identical to before — no runtime behavior changes. jsonCreate/jsonUpdate and the database variants keep their existing OptionFromOptionalNullOr/OptionFromNullOr mappings.

This is a type-level breaking change for the published effect-firebase package: the json Encoded type for composed Optional(Field) / OptionalNull(Field) no longer admits null. The only affected code is the (undocumented) pattern of annotating a null literal against typeof Model.json.Encoded for such a field — which compiled before but already threw SchemaError at runtime, so the fix converts a runtime failure into a compile-time error. The encoding direction, jsonCreate/jsonUpdate Encoded types, the simple forms, and OptionalDeletable are unaffected. Documented in MIGRATION.md per the repo's breaking-change convention, and flagged with a BREAKING CHANGE commit footer for the release notes.

Testing

• Type-level regression guard (committed): added a composed json null-admission block to optional.spec.ts using typed identity-function helpers with @ts-expect-error directives (the repo's existing type-assertion pattern, e.g. query.spec.ts, repository.spec.ts). The block covers all five composed forms across both a 6-variant Field (Reference) and a 4-variant Field (DateTime), with OptionalDeletable as a control. With the fix applied the directives are all consumed; reverting the fix produces exactly 4× TS2578 "Unused '@ts-expect-error' directive" at the bug-affected forms while the OptionalDeletable control directive stays consumed — i.e. the test fails to compile iff the divergence returns. Positive no-regression checks assert jsonCreate/jsonUpdate Encoded types still admit null at both the type and runtime levels.
• Runtime: Schema.decodeUnknownSync(<Model>.json)({ name: 'x', <field>: null }) throws for all five composed forms (unchanged by the type-only fix), and jsonCreate/jsonUpdate still decode null to Option.none().
• Routine checks pass: full effect-firebase vitest suite (308 tests across 13 spec files), an isolated tsc --noEmit typecheck of the changed module and its in-package dependencies (exit 0, all directives consumed), eslint (0 errors; the 6 no-explicit-any warnings are pre-existing as any casts untouched by the fix), prettier, and pnpm nx run-many -t build across all 8 workspace projects (no downstream breakage via the @effect-firebase/source source-export condition).
• Pre-existing, unrelated: pnpm nx typecheck effect-firebase (the spec-project typecheck target) is red in the baseline before this change — 29 errors from TS6307 "File is not listed within the file list of project" across source files imported by specs, plus unrelated transaction.spec.ts/reference.spec.ts/query.spec.ts issues. Stashing the fix and re-running on a clean tree produced the identical 29 errors with the identical breakdown, confirming this change adds no new errors; the authoritative type-level verification is the isolated tsc run above.

---

Automatic Fixes PRs can be configured here.

[greptile-apps]

Confidence Score: 4/5

The implementation appears safe to merge, with the non-blocking caveat that its type-level regression guard is not enforced by the current CI pipeline.

Findings

1.  Type Guard Is Unenforced ▶

Summary

This PR aligns the composed Optional and OptionalNull conditional return types with their existing runtime schemas by rejecting null from the normal json variant while preserving its acceptance in jsonCreate and jsonUpdate.

• Special-cases the json variant as Schema.OptionFromOptional.
• Adds runtime and compile-time coverage for composed reference and date fields.
• Documents the resulting type-level breaking change in the migration guide.
• The compile-time regression assertions are not currently enforced by committed CI.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  F[Composed Optional Field] --> V{Model variant}
  V -->|json| O[OptionFromOptional]
  O --> R[Missing key accepted; null rejected]
  V -->|jsonCreate or jsonUpdate| N[OptionFromOptionalNullOr]
  N --> A[Missing key and null accepted]
  V -->|database variant| D[Existing database mapping retained]

Loading

_{Reviews (1) · Last reviewed commit: "fix(firestore)!: narrow composed Optiona..."}

[greptile-apps]

Type guard is unenforced

The new @ts-expect-error assertions are not checked by the committed CI path. CI runs lint, test, and build; Vitest transpiles these specs without type-checking them, and the build configuration excludes *.spec.ts. Reverting the conditional-type fix would therefore leave these directives unused without failing CI, so this regression guard is ineffective. Please add a working type-check target to CI or move the assertions to a type-test surface that CI compiles.

[detail-app]

Confirmed the guard is unenforced as you noted, but the fix is a CI/build-config change outside this PR's diff, so I'm leaving it for a maintainer to decide.

What I checked:

• CI runs nx affected -t lint test build only (.github/workflows/ci.yml:51); no typecheck.
• build uses tsconfig.lib.json, which excludes src/**/*.spec.ts (tsconfig.lib.json:14-27), so this spec isn't compiled by build.
• test runs vitest, which transpiles specs without type-checking, so the @ts-expect-error directives are not validated.
• The only existing type-check path is the typecheck target, and it is already broken: nx run effect-firebase:typecheck fails with 29 TS6307 errors because tsconfig.spec.json's composite project doesn't list the lib sources it imports (via repository.spec.ts, query.spec.ts, array.spec.ts, datetime.spec.ts — none of which are changed by this PR; the tsconfig*.json files are unchanged vs main). That looks like exactly why typecheck isn't wired into CI.

Enforcing the guard therefore needs either (a) fixing tsconfig.spec.json's project setup and adding typecheck to ci.yml, or (b) enabling vitest typecheck in vite.config.mts — all files outside this PR's diff. The runtime regression tests in this spec (the "json runtime rejects null" block) are already enforced by the test target; only the compile-time @ts-expect-error directives aren't. A human should decide whether to take the infra change in this PR or a separate one.