feat(TSchema): add explicit flat option for Union members#52
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a "flat" option to TSchema.Struct for more efficient CBOR encoding in unions. When a struct is marked as flat (or has a custom index), it skips the nested constructor wrapping, resulting in single-level encoding Constr(index, [fields]) instead of double-nested Constr(unionPos, [Constr(index, [fields])]).
Key changes:
- Added
StructOptionsinterface withindexandflatproperties - Modified
Structto accept options and store metadata in annotations - Updated
Unionto detect flat members and handle encoding/decoding accordingly - Added collision detection to prevent index conflicts between flat and nested members
Reviewed Changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/evolution/src/core/TSchema.ts | Added StructOptions interface and updated Struct and Union to support flat encoding |
| packages/evolution/test/TSchema-flat-option.test.ts | Comprehensive test suite covering default behavior, flat options, collision detection, and edge cases |
| packages/evolution/docs/modules/core/TSchema.ts.md | Updated documentation for Struct and Union with new options and examples |
| Multiple documentation files | Updated navigation order and improved documentation descriptions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { flat, index = 0 } = options | ||
|
|
||
| // Default: flat is true when index is explicitly set, false otherwise | ||
| const isFlat = flat ?? (options.index !== undefined) |
There was a problem hiding this comment.
The logic for determining isFlat has an issue: when flat is explicitly set to false and index is provided, the code should respect the explicit flat: false setting. However, the current implementation uses ?? which only checks for null or undefined, so flat ?? (options.index !== undefined) would correctly return false when flat is explicitly false. However, this logic is confusing. Consider using: flat !== undefined ? flat : (options.index !== undefined) for clarity.
| const isFlat = flat ?? (options.index !== undefined) | |
| const isFlat = flat !== undefined ? flat : (options.index !== undefined) |
| }) | ||
| } | ||
| // Return the encoded Constr as-is (it already has the custom index) | ||
| return encodedValue |
There was a problem hiding this comment.
The encoding logic for flat members doesn't validate that the encoded value is a Data.Constr before accessing encodedValue.fields. If a non-Constr value is encoded and marked as flat, the check encodedValue instanceof Data.Constr prevents issues, but if it fails, the code falls through to the nested encoding path. This could lead to unexpected behavior. Consider adding an else branch to handle the case where a flat member doesn't encode to a Constr, or add a more explicit error.
| return encodedValue | |
| return encodedValue | |
| } else if (memberInfo.isFlat && !(encodedValue instanceof Data.Constr)) { | |
| throw new Error( | |
| `Flat union member at position ${memberInfo.position} did not encode to a Data.Constr. ` + | |
| `Encoded value: ${JSON.stringify(encodedValue)}` | |
| ); |
| memberInfos.forEach((flatMember, flatPos) => { | ||
| if (flatMember.isFlat) { | ||
| const flatIndex = flatMember.customIndex ?? flatMember.position | ||
|
|
||
| // Check if this flat member's index conflicts with any non-flat member's position | ||
| memberInfos.forEach((otherMember, otherPos) => { | ||
| if (!otherMember.isFlat && flatIndex === otherMember.position) { | ||
| collisions.push({ | ||
| autoPosition: otherPos, | ||
| conflictingIndex: flatIndex, | ||
| customPosition: flatPos | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The collision detection logic only checks for collisions between flat members and nested members, but it doesn't detect collisions between two flat members with the same index. Two flat structs with { index: 100, flat: true } would both encode to Constr(100, [...]), causing ambiguity during decoding. Add logic to detect flat-to-flat collisions.
Changes
Adds `flat?: boolean` option to `TSchema.Struct` for explicit control over Union unwrapping behavior.
Implementation
Patterns Enabled
Use Cases