Repository navigation
[2.x] Add withPath and visitPath helpers to NodeStack - #1186
Conversation
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds NodeStack.withPath(path, cb) and NodeStack.visitPath(path, visitor) to @codama/visitors-core and replaces the five hand-rolled pushPath/try/finally/popPath blocks across getByteSizeVisitor, getMaxByteSizeVisitor, getInstructionDataFields, unwrapDefinedTypesVisitor and dynamic-codecs with them. visitPath also fixes a subtle inaccuracy: the old callers pushed the full linked path and then visited its last node through a recordNodeStackVisitor-wrapped visitor, so the linked node appeared twice on the current path. It now appears once. New tests cover both helpers, including the throw/restore path.
The implementation is straightforward and I verified each call site picks the right helper: the three that visit the linked node itself through a stack-recording visitor use visitPath, and the two that descend into definedType.type directly (where the defined type genuinely belongs on the path) use withPath. No circular import is introduced (visitor.ts has no local imports), and the remaining visit / getLastNodeFromPath imports in the touched files are all still used.
Things to address
Missing changeset. CONTRIBUTING.md asks for a changeset for any user-facing change (npx changeset add --empty). This PR adds public API to @codama/visitors-core (which versions in the fixed core group) and changes the observable NodeStack contents while sizing/encoding a linked type in @codama/dynamic-codecs. I'd expect at least a minor for the new NodeStack methods; whether the dedup deserves its own patch entry for dynamic-codecs is your call, but it's worth a line since anyone reading stack.getPath() from a shared stack inside those visitors will see a different (correct) path.
README. packages/visitors-core/README.md → "Recording node paths" → NodeStack currently documents pushPath/popPath specifically for the "jump to a linked node and back" use case. Since withPath/visitPath are now the recommended way to do that, they should be added there (and the existing pushPath/popPath paragraph could point at them).
Notes for other reviewers
- The behavioural change (linked node recorded once instead of twice) is intentional and only affects consumers that read the stack while inside a linked type via a shared
NodeStack.linkables.getPath(...)resolution relies onfindProgramNodeFromPath, which is unaffected by the duplicate, so link resolution behaves identically before and after. visitPath's type signature (Visitor<TReturn, TNode['kind']>whereTNodecomes fromNodePath<TNode>) accepts wider visitors, which is what makes thedynamic-codecscall with aVisitor<Codec, EncodableNodes['kind']>type-check against the linkable union.
4a741b7 to
27b430c
Compare
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Both follow-ups from the previous pass landed:
- README — the
NodeStacksection now documentswithPath/visitPathright after thepushPath/popPathexample, including the "visitor must record on the same stack, otherwise usewithPath" caveat. Reads well and matches the JSDoc on the methods. - Test nit — the
visitPathtest now hasrecordNodeStackVisitorlast in the pipe and asserts[root, root.program, slot, slot.type], which is a much clearer demonstration of the "recorded once" property.
The implementation itself is unchanged and still looks correct.
Still outstanding
Changeset. The changed-files list still contains nothing under .changeset/. CONTRIBUTING.md requires one for user-facing changes and this adds two public methods to @codama/visitors-core (fixed core group), so a minor entry is needed before merge. Optionally a patch line for @codama/dynamic-codecs noting the linked node is no longer duplicated on the stack.
That's the only blocker; happy to approve once the changeset is in.
ffe14c7 to
b926e05
Compare
cbf5907 to
d770279
Compare
b926e05 to
8d5b927
Compare
d770279 to
61564be
Compare
1416323 to
dfad4fa
Compare
61564be to
c2a6daa
Compare
dfad4fa to
d814828
Compare
deab972 to
96364bb
Compare
8a0ec75 to
ed7bb21
Compare
96364bb to
be3aa52
Compare
ed7bb21 to
7cf264c
Compare
be3aa52 to
0bd6cdb
Compare
7cf264c to
ea8ce7d
Compare
0bd6cdb to
97392e5
Compare
ea8ce7d to
22d0df8
Compare
97392e5 to
690ceb0
Compare
22d0df8 to
26f1b77
Compare
0c50243 to
6218f87
Compare
26f1b77 to
027a158
Compare
6218f87 to
655bc94
Compare

This PR adds two helpers to
NodeStackin@codama/visitors-corefor jumping to another part of the tree, e.g. to the definition of a linked node.withPath(path, callback)runscallbackwithpathas the current path and restores the previous one, even ifcallbackthrows.visitPath(path, visitor)visits the last node ofpathwith the rest ofpathas the current path. The visited node is therefore recorded once, rather than twice as when the full path was pushed before visiting it.The existing
pushPath/popPathpairs ingetByteSizeVisitor,getMaxByteSizeVisitor,getInstructionDataFields,unwrapDefinedTypesVisitoranddynamic-codecsnow use these helpers.