Repository navigation
[2.x] Add v1 to v2 upgrade converters for types and values - #1200
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds the v1 → v2 converters for types, values, displays, links, PDAs, discriminators and definitions under packages/upgrade/src/v1ToV2/, without wiring them into upgrade() yet. Nodes are built as plain frozen objects via compactAndFreeze (so the step keeps emitting v2 once @codama/nodes moves on), v1 numbers are typed from their expected type by following links across programs through a V1NodePath abstraction, wrappers become transforms, and the quantity/date-time/duration display → type migrations are handled with explicit lossy rules. Also fixes the existing upgrade tests that assumed v1 was the latest major, and adds a type-only src/v2 namespace that re-exports @codama/node-types for now.
This is a careful, well-tested PR. The path-based link resolution in paths.ts is clean, the lossy cases are explicitly enumerated and tested, and the freeze-everything test in shared.test.ts is a nice guard. Comments below are small; the only one I'd want addressed before merge is the first.
Things to watch out for
-
dateTimeTypeNodeFromV1can leak a display into the date-time'snumberslot.numberTypeNodeFromV1keepsamountNumberDisplayNodes on integers, so a v1dateTimeTypeNodewhose inner number carries one yields a v2dateTimeTypeNodewhosenumberhas adisplay— which the v2DateTimeTypeNode.numberdocblock forbids ("a pure encoding slot… must not carry aunitordisplayof its own").quantityTypeNodeFromV1already strips displays for exactly this reason; this path should too. Inline comment with details. -
Duration display inside a v1
dateTimeTypeNodedrops the date-time entirely. Thedefaultbranch returnsconverted.numberfor thedurationTypeNodecase, so the result is a bare integer. I'd have expected the outer, type-leveldateTimeTypeNodeto win over a presentational display. Question inline; either way it's untested and missing from the lossy table in the description. -
Non-integral numbers on integer types become
floatValueNodes (numberValueNodeFromV1). That's lossless and tested, but it produces a v2 tree the validators will reject rather than throwing at upgrade time likeintegerTypeNodeFromV1does for float formats. I think not throwing is the right call for value nodes (the v1 tree was already invalid), just flagging the asymmetry so it's a conscious choice — worth a line in the description. -
CONTRIBUTING playbook. Step 1 of "Releasing a new major" says to snapshot
@codama/node-types/src/generatedintopackages/upgrade/src/vN/generated. Now thatsrc/v2/index.tsexists as a living re-export of@codama/node-types, cutting v3 means replacing that file with the frozen snapshot's index and creatingsrc/v3/index.tsas the new re-export. The docblock insrc/v2/index.tssays this, but the playbook is meant to be the single source of truth — a sentence there (and maybe a note that@codama/node-typesindependenciesis type-only, to reconcile with the "zero runtime dependencies" invariant) would keep it that way. Could be a follow-up whenupgrade()gets wired.
Notes for subsequent reviewers
- Enum value link resolution is positional.
enumValueNodeFromV1resolvesvalue.enumfrom the value's path (the program the value is written in), not from the path of the type the value belongs to. The test "it resolves enum links without a program in the program of the value" pins this. It matches v1LinkableDictionarysemantics, so it looks right to me, but it's the subtlest decision invalueNodes.ts. - 1-tuple variants are deliberately kept as tuples, even though v2 says a single-type variant needs no tuple. The description explains why (faithful translation, generated APIs unchanged). Valid v2, just non-idiomatic.
unitvsunitNumberDisplayNodeon floats:amountTypeNodeover a float →unit(static string, semantic), while anamountNumberDisplayNodeover a float →display: unitNumberDisplayNode(injectable, presentational). That asymmetry is correct given v1's two differentunitfields, not an inconsistency.displayNodeFromV1shallow-spreads v1 display nodes; I verified all five "unchanged" kinds are flat (strings/enums only) in v1 and that v2 acceptsstringwhere it now also allowsTextNode, so this is safe.decimalStringFromV1/integerStringFromV1outputs match theDecimalString/IntegerStringgrammars inbrands.ts, including-0,NaNand±Infinity.
74545e7 to
cd53890
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
All three code-level points from my first pass are addressed, and the description now documents both lossy behaviours I asked about. Approving.
What changed since last time (typeNodes.ts, typeNodes.test.ts, description):
dateTimeTypeNodeFromV1now rebuilds thenumberslot withdisplay: undefinedfor both theintegerTypeNodeanddurationTypeNodecases, so an amount display can no longer leak into the v2 date-time's pure encoding slot. The two branches share one arm, which keeps it tidy. ThedateTimeTypeNodebranch is safe without stripping becausenumberTypeNodeFromV1only attaches a display when the v1 display is anamountNumberDisplayNode, which is mutually exclusive with the date-time display that produces that branch.- A duration display inside a v1
dateTimeTypeNodenow yields adateTimeTypeNode(type wins over presentational display) rather than a bare integer. The new test covers both the amount and duration cases, and the duration case also exercises a wrapper so the transforms-on-the-date-time path is pinned. - The docblock on
dateTimeTypeNodeFromV1spells out the rules, and the description's lossy table gained the corresponding row plus the note on non-integral values becomingfloatValueNodes.
Still open, non-blocking: the CONTRIBUTING "Releasing a new major" step 1 doesn't yet mention that src/v2/index.ts is a living re-export that gets replaced (and src/v3/index.ts created) at the next major. Fine as a follow-up when upgrade() is wired, as noted before.
No new issues found in the re-fetched diff.
cd53890 to
f1c5fa8
Compare

This PR adds the converters that turn v1 type and value nodes into v2 ones, in
packages/upgrade/src/v1ToV2/. It doesn't wire them intoupgrade()yet: that comes with instructions, programs and the root.What they do:
nametoidentifier, joins docs with\n, and turns wrapper types into transforms.enumVariantTypeNode. One-item tuple variants keep their tuple, along with the payloads of their values, so the upgrade is a faithful translation and generated APIs stay unchanged.3on anf64becomesfloatValueNode('3'). Without a type, whole numbers become integers.programNode.originis dropped.Nodes are built as plain objects with
compactAndFreezerather than with the@codama/nodesconstructors. That way this step keeps producing v2 nodes once the constructors move on to later majors. Each node is frozen along with its array attributes, and its attributes mirror the v1 node rather than the v2 constructor defaults.Some v1 information can't be expressed in v2 and is dropped. None of it affects encoding:
amountTypeNodeon a floatamountTypeNodeonshortU16dateTimeTypeNodeA float where only integers are accepted, such as a size prefix, throws. A non-integral value on an integer type, such as
1.5on au64, becomes afloatValueNoderather than throwing: the v1 input was already invalid, and this is the one case where the output isn't valid v2, so validators will report it.This PR also fixes the existing upgrade tests, which assumed v1 was the latest major, and corrects the README, which claimed the v1 types were generated.