feat(core): add discriminated union (sum type) support#76
Draft
typedrat wants to merge 10 commits intoloro-dev:mainfrom
Draft
feat(core): add discriminated union (sum type) support#76typedrat wants to merge 10 commits intoloro-dev:mainfrom
typedrat wants to merge 10 commits intoloro-dev:mainfrom
Conversation
- Add resolveUnionVariantSchema() helper in diff.ts that resolves a union schema to the concrete variant's LoroMapSchema based on the discriminant value in the data - Handle variant switches in diffListWithIdSelector and diffMap by detecting discriminant changes and emitting delete+insert instead of recursive field updates (prevents CRDT chimera states) - Update mirror.ts initializeContainers() to include "loro-union" in the container type list for root registration - Resolve union→variant in initializeContainer() so fields are properly set on the underlying LoroMap - Handle unions in registerNestedContainers() and getSchemaForChild() so nested containers within union variants get proper schemas - Add "loro-union" handling in applyRootChanges() and mergeInitialIntoBaseWithSchema() - Add 5 Mirror integration tests covering: initial state via setState, same-variant field updates, variant switching, multiple union items, and root-level union fields
- Nested containers inside union variants (LoroText) - Single-variant unions - Adding new items to a list of unions - Removing union items from a list via splice - Public API export verification (LoroUnionSchema, isLoroUnionSchema)
- Add schema.Union to builder lists in CLAUDE.md/AGENTS.md, api.md, README.md, and packages/core/README.md - Add LoroUnionSchema to re-exported types lists - Add isLoroUnionSchema to type guard lists - Add union code example to api.md Schema Builder examples - Note $cid presence in union variants
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6514192dd3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Previously, defineCidProperty bailed out if $cid already existed on the target, regardless of how it was set. This is correct for properties stamped by the mirror itself (non-configurable), but wrong when a user passes an explicit $cid in their state object (e.g., to match items by identity during variant switches). In that case the user-set $cid is a regular configurable+enumerable property with the OLD container's id, and the mirror needs to replace it with the NEW container's id after creating the replacement. The fix distinguishes the two cases via the configurable flag: - Non-configurable (stamped by us previously): skip, value is current - Configurable (user-set or first stamp): overwrite with the correct non-enumerable value New properties are created with configurable: true so future overwrites work. This is safe because $cid is already non-enumerable (hidden from Object.keys/JSON.stringify/deepEqual), so the only code path that touches it is defineCidProperty itself.
When diffListWithIdSelector detects a union variant switch on a kept item (phase 3), the delete used oldInfo.index — the item's position in the original array. By the time this change is applied, phase-1 deletes and phase-2 inserts have already shifted the list, so the item is at newInfo.newIndex. Deleting at the stale index removes the wrong element, corrupting the Loro list. Use newInfo.newIndex for the delete, matching the pattern already used by the non-union fallback path (line ~902).
…rding it
Two issues combined to make initialState ineffective for union fields:
1. mergeInitialIntoBaseWithSchema coerced union fields to {} without
merging the provided initialState data. Fixed by resolving the
variant from the discriminant value, setting it directly (since it's
not part of the variant's schema definition), and recursing with the
variant's map schema — matching the pattern used by the loro-map
branch.
2. initializeContainers used Object.assign to overlay the doc snapshot
onto this.state, which shallow-replaced nested objects and wiped out
any data merged in step (1). Replaced with deepMergeSnapshot, which
walks both trees and stamps $cid metadata from the snapshot without
overwriting existing content. This preserves initialState data for
all container types while still injecting $cid on initial snapshots.
Also cleans up two tests that worked around the bug by using setState
with `as Record<string, unknown>` casts — they now use initialState
directly.
Author
|
@codex review -- you should recheck after I fixed the bugs you previously pointed out. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0013ae156
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…dev#76) - Replace isObjectLike with isObject at union guard sites in diff.ts to prevent null dereference (isObjectLike returns true for null) - Add configurable: true to restoreCidDescriptors so $cid can be redefined after snapshot restoration - Validate that variant definitions don't contain the discriminant key, both at schema creation time and in validateSchema - Remove incorrect fallback in resolveUnionVariantSchema that returned the parent union schema for unknown tags - Use tryUpdateToContainer instead of insertChildToMap for map-level variant switches so container type is resolved from schema - Resolve union schema to active variant in updateMapContainer so nested container fields are created correctly - Guard mergeInitialIntoBaseWithSchema to preserve existing doc discriminant over initialState - Persist union discriminants to Loro doc during Mirror construction when initialState provides them and the doc doesn't already have them
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #75
Adds
schema.Union(discriminant, variants, options?)— a first-class way to define discriminated unions where each variant is aLoroMapSchema. The discriminant key (e.g."type") gets auto-injected into TypeScript types, so you get proper narrowing out of the box.At the Loro level these are just
LoroMapcontainers. When the discriminant changes (variant switch), the entire container is replaced to avoid chimera states. Same-variant updates diff fields incrementally as usual.Changes
LoroUnionSchemainterface and type inference helpers (InferUnionType,InferInputUnionType)schema.Union()builderisLoroUnionSchematype guard, validation and default value supportapi.md,README.md, andpackages/core/README.mdTest plan
Full disclosure: the actual code was written partly by myself and partly by Claude Opus 4.6, and Opus 4.6 assisted me in understanding a plan of attack to implement the feature in the first place. However, the test coverage is comprehensive enough that I'm fairly confident that it does indeed work correctly, and I wrote this in no small part because it was needed for a work project, which will provide a real-project proving ground for the feature.