Repository navigation
[2.x] Make NodePath non-distributive over node unions - #1205
lorisleiva wants to merge 1 commit into
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Makes NodePath<TNode> non-distributive over the node type: NodePath<A | B> is now a single readonly [...Node[], A | B] tuple instead of NodePath<A> | NodePath<B>. The trick is the NodePathImpl<TCheck, TNode> indirection: the conditional still distributes over TCheck (so undefined members still resolve to readonly Node[] and generic TNodes stay deferred rather than collapsing), but the tail element reads from the second, non-checked parameter, which TS does not substitute per-member. Exclude<TNode, undefined> is needed precisely because TNode is the whole union in that branch. Adds a type-level test file.
I walked through the four cases (undefined, single kind, union, X | undefined) plus the generic TLinkNode path in LinkableDictionary and the predicate narrowing in recordPath (isNodePath(linkablePath, 'accountNode') now narrows a single tuple type to a subtype rather than filtering a union — assignable, so it still works). The implementation is correct and the doc comments explain the non-obvious parts well.
A nice side effect worth knowing about: NodePath<Node> used to expand to a ~70-member union of tuple types; it's now one tuple. That should help both compile times and error messages wherever isFilledNodePath / NodePath<Node> show up.
Before merging
- Changeset.
CONTRIBUTING.mdasks for a changeset on any user-facing change, and this changes a public exported type with a consumer-visible caveat (parameters typedNodePath<A> | NodePath<B>no longer accept aNodePath<A | B>argument). Apatchfor@codama/visitors-core(the fixed core group handles the rest) with a one-liner mirroring the PR description seems right. If the 2.x branch policy deliberately skips changesets for type-only changes, ignore this.
Notes for other reviewers
- The only possible regression is the one named in the description: any parameter typed as an explicit union of paths. Nothing in
visitors-coredoes that, andpnpm test:typesacross the monorepo is the real guard here. It's worth a quick grep forNodePath<\w+> \|in the downstream renderers (renderers-js,renderers-rust,renderers-core) before the next release, since astack.getPath(['accountNode', 'definedTypeNode'])result would now be rejected by such a parameter. - Inference of
TNodeingetLastNodeFromPath/NodeStack.visitPathnow goes throughExclude<TNode, undefined>instead of a bareTNodein the tuple tail. The test ongetLastNodeFromPath(path)returningAccountNode | PdaNodeconfirms inference still resolves through it.
6f0a4f9 to
4f94500
Compare

This PR stops
NodePathfrom splitting node unions into one path type per member:The new type is strictly wider, so existing paths still type-check. It also accepts paths built from a node only known as a union, such as
[...stack.getPath(), node], which previously didn't compile againstNodePath<Node>. The one thing it no longer does is fit a parameter typed as an explicit union of paths, e.g.NodePath<AccountNode> | NodePath<PdaNode>. Such parameters should useNodePath<AccountNode | PdaNode>instead.The conditional still distributes over the node type to tell
NodePath(any list of nodes) apart fromNodePath<T>, but every path now ends at the whole node type. A plain[TNode] extends [undefined]check would have leftNodePath<T>unresolved for generic node types, e.g. inLinkableDictionary.undefinedis excluded from the last node, soNodePath<PdaNode | undefined>remainsreadonly Node[] | readonly [...Node[], PdaNode].A new
NodePath.test.tscovers union, single-kind, untyped and optional paths. Its union assertion fails to compile with the previous type.