Unify enum variants into a single enumVariantTypeNode - #138
Merged
Conversation
The three enum variant flavours collapse into one enumVariantTypeNode whose payload is an optional data type node: absent for unit variants, a struct for named fields, a tuple for positional fields, and any other type carried as-is — a variant holding a single type needs no tuple around it. The node gains an optional docs attribute matching structFieldTypeNode, the enumVariantTypeNode union is deleted (the name now identifies the node), and enumTypeNode.variants plus registeredTypeNode reference the node directly.
Member
Author
trevor-cortex
approved these changes
Sep 1, 2026
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Implements #127: collapses enumEmptyVariantTypeNode, enumStructVariantTypeNode, and enumTupleVariantTypeNode into a single enumVariantTypeNode with an optional data: TypeNode payload, deletes the now-redundant enumVariantTypeNode union, and adds an optional docs attribute per the #124 resolution. Docs, spec.json, examples, and tests are all updated in lockstep.
The execution is thorough and consistent:
- The new node definition mirrors
structFieldTypeNodeconventions exactly — samedocs()usage, same doc wording ("Markdown documentation for the…"), and thename → discriminator → docs → data → displayordering matches the established data-before-children pattern (cf.accountNode). - The union removal is applied everywhere it needs to be:
TypeNode.ts,ALL_TYPE_NODE_UNIONS,registeredTypeNodemembers, the test's principal-unions list, and the docs README's Unions section. - The new test block covers the shape assertion, the
dataoptionality/union type/child-attribute status, and explicitly asserts the three old nodes and the old union are gone — good regression coverage. - Changeset is correctly
majorper the repo's semver policy, with a clear migration diff.
Notes for subsequent reviewers
- The generated artifacts (
spec.json,docs/) look consistent with the source changes; CI verifies lockstep so no manual diffing needed there. - The one thing worth a second opinion is the upgrade guidance's blanket 1-tuple unwrap rule — see my inline comment on the changeset. When the single tuple item is itself a
tupleTypeNodeorstructTypeNode, unwrapping is wire-compatible but changes the variant flavour a renderer would emit (e.g.V(SomeInlineStruct)becomesV { … }). Whether that's acceptable or should be carved out is a judgement call for theupgradeV1ToV2implementation, but the changeset text is where consumers will look for the rule, so it's worth being precise there. - Minor: the migration example in the changeset (and the
EnumTypeNodeexample) still wraps a singleu32in a 1-tuple for therotatevariant, right after the prose recommends unwrapping exactly that shape. Both spellings are valid v2, so this is cosmetic — inline comments below.
Carves struct, tuple and defined-type-link items out of the single-item tuple unwrap rule — those shapes determine the variant flavour in data position, so unwrapping them would change the generated API despite identical wire format. The migration diff and the flagship enum example now model the recommended unwrapped output for a plain single-type payload.
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
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.
This PR implements #127: the three enum variant flavours collapse into one
enumVariantTypeNodewhose payload is an optionaldata: TypeNode— absent for unit variants, a struct for named fields, a tuple for positional fields, and any other type node carried as-is (a variant holding a single type needs no tuple around it).As scoped in the #124 resolution, the node gains an optional
docsattribute matchingstructFieldTypeNode, so enum variants (doc-comment carriers in Rust) are documentable. Attribute order follows the convention:name,discriminator,docs, then thedata/displaychildren.The
enumVariantTypeNodeunion is deleted — the name now identifies the node — withenumTypeNode.variantsandregisteredTypeNodereferencing the node directly. Examples rewritten; counts move to 93 nodes / 33 unions.Upgrade lens note (for
upgradeV1ToV2): mechanical — empty variant → nodata; struct variantstruct→data; tuple varianttuple→data, except that a tuple holding exactly one item unwraps to that single type, unless the item is astructTypeNode,tupleTypeNodeordefinedTypeLinkNode— those shapes determine the variant flavour indataposition, so unwrapping would change the generated API (links are kept wrapped so the lens never needs resolution to be correct). The 1-tuple was the v1 workaround spelling of a single-type payload, and this unification is the proper solution. Renderers whose target requires a wrapper (e.g. the Rust renderer emitting a newtype-style variant) simply re-wrap on their side.name/discriminator/displaycopy through.Closes #127 (manual close — default branch is
1.x). Part of #102.