Repository navigation
fix(traverse): pass through filtered nodes; add exports map and files allowlist - #2
Merged
Merged
Conversation
… allowlist
Two fixes found while graphbase was being used vendored inside another
repository, ported back so the standalone package is the source of truth.
traverse(): the nodeTypes filter pruned the traversal instead of filtering the
result. The code continued before expanding the node's edges, so nothing beyond a
filtered node was ever reachable -- directly contradicting the comment sitting on
that branch, which said filtered nodes should be reachable through. It also
continued before marking the node visited, so a filtered node reachable by two
paths was dequeued and reprocessed on every path that reached it.
Now the node is marked visited and its edges are expanded exactly as for any
other node; only the push into the result array is gated. A caller who wants
prune semantics cannot reconstruct pass-through, whereas a caller who wants only
the listed types can already filter the returned array, so pass-through is the
direction that keeps both options open.
The existing "should filter by node type" test could not catch this: every
non-matching node in its fixture is a leaf, so pruning and passing through give
the same answer. The two tests added here do distinguish them -- a three-hop
A(Person) -> X(Company) -> B(Person) path, and a filtered node reachable by two
paths asserted via a getNode spy to be looked up exactly once. Both fail against
the previous implementation (2 failed / 356 passed) and pass with it (358).
package.json: add an exports map, module, files and sideEffects. The package
declared type: module with main pointing at the ESM build and no exports map, so
the CJS output tsup was already producing was unreachable and require('graphbase')
threw ERR_REQUIRE_ESM on Node 20 (Node 22.12+ masks this via require(esm), which
is why it can look fine locally). With no files allowlist, npm publish also
shipped the entire working tree; the allowlist brings a pack from 52 files to 6.
Upstream's build/check:dts scripts are left as they are.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019guyDrzCGThrLyAR8Yqm8R
…pty package Adding `files: ["dist"]` in the previous commit narrowed the published tarball from the whole working tree to the build output, which is what we want -- but dist/ is gitignored, and nothing guaranteed it existed at pack time. On a clean checkout `npm publish` would have produced a tarball containing only package.json and README: strictly worse than the over-broad publish it replaced, because it fails at import rather than merely shipping extra files. The six-file pack measured in the PR description was taken after a manual build, so it did not expose this. CI builds but never packs, so it would not have caught it either. prepack runs for both `npm pack` and `npm publish`, so the artifacts the exports map points at are always present in the tarball. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019guyDrzCGThrLyAR8Yqm8R
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.
Two fixes found while
graphbasewas being used vendored insidecodegraph-de,ported back here so the standalone package stays the source of truth.
1.
traverse()— thenodeTypesfilter pruned instead of filteringtraverse.tscontinued before expanding the node's edges, so nothing beyond afiltered node was ever reachable — directly contradicting the comment on that very
branch, which said filtered nodes should be reachable through. It also
continuedbefore
visited.add, so a filtered node reachable by two paths was dequeued andreprocessed on each path.
Now the node is marked visited and its edges expanded like any other; only the
result.pushis gated.Why pass-through rather than prune: a caller who wants prune cannot reconstruct
pass-through, whereas a caller who wants only the listed types can already filter the
returned array. Pass-through is the direction that keeps both options open.
The existing test could not catch this. In
operations.test.ts's fixture everynon-
Personnode is a leaf, so prune and pass-through give identical answers. The twotests added here do distinguish them:
A(Person) → X(Company) → B(Person)path, assertingBis reachedgetNodespy to be looked upexactly once (the re-enqueue regression)
Verified against this branch: 2 failed / 356 passed without the fix, 358 passed with it.
2.
package.json— the CJS build was unreachable, and publish shipped everythingtype: module+mainpointing at the ESM build + noexportsmap meant the CJSoutput
tsupalready produces was unreachable:require('graphbase')threwERR_REQUIRE_ESMon Node 20. Node 22.12+ masks this viarequire(esm), which is whyit can look fine locally — verified both ways with
--no-experimental-require-module.Adds
exports(per-condition types),module,filesandsideEffects. With nofilesallowlistnpm publishshipped the whole working tree; the allowlist takes apack from 52 files to 6.
Upstream's
build/check:dtsscripts are untouched.Verification
tsc --noEmitclean ·npm run build(incl.check:dts) clean · 358/358 tests ·npm pack --dry-run= 6 files · CJSrequire()verified under Node 20 semantics.🤖 Generated with Claude Code
https://claude.ai/code/session_019guyDrzCGThrLyAR8Yqm8R
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.