Repository navigation
fix(build): strip the react-native tree from the published shrinkwrap - #1959
Merged
Merged
Conversation
alasql declares `optionalDependencies: { "react-native-fs": "^2.20.0" }`, and
react-native-fs peer-depends on react-native without marking it optional. npm 7+
auto-installs peer deps, so resolving alasql drags in react-native, react,
hermes, metro and react-devtools-core: ~140MB and ~200 packages that cannot
execute under Node, since every require('react-native-fs') in alasql sits behind
an isReactNative guard. Harper only uses alasql.parse and its function
extensions. The published 5.1.23 shrinkwrap pins 794 packages, 64 of them
react-native/hermes/metro.
Pruned from the published shrinkwrap rather than via an `overrides` entry,
because npm honours overrides only for the root project — they do nothing for
anyone installing harper. The published shrinkwrap is authoritative for registry
installs: npm learns it exists from the packument's `_hasShrinkwrap` flag and
installs exactly the tree it describes, without re-resolving pruned optional
dependencies. Verified against npm 11 by serving a package with a pruned
shrinkwrap from a local registry — 331 packages became 23, with no react-native
tree and no react-native-fs node. So this needs no stub package and no
third-party empty module.
The prune is surgical by construction: it computes the packages reachable with
and without the react-native-fs edge and removes only the difference, so nothing
reachable by another path can be removed, and the set is derived rather than a
hardcoded list of directory names that would rot as alasql's tree shifts. On the
current tree it removes 262 entries and leaves zero unresolved required edges.
Runs alongside prune-shrinkwrap-dev.mjs, which already enforces the same
invariant (#1783).
Refs #1937
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a build tool script prune-shrinkwrap-react-native.mjs along with its corresponding unit tests and documentation in dependencies.md. The script surgically prunes the react-native-fs dependency tree from the published npm-shrinkwrap.json to prevent transitive peer dependencies like react-native and react from being installed by consumers. There are no review comments, and I have no feedback to provide.
Contributor
|
Reviewed; no blockers found. |
heskew
marked this pull request as ready for review
July 28, 2026 03:28
Captures why the react-native prune lives in build-tools rather than in `overrides`, and why the Docker image gets none of it: npm honours a bundled npm-shrinkwrap.json only for registry installs, keyed off the packument's `_hasShrinkwrap` flag, and re-resolves from package.json for tarball installs. Refs #1937, #1960 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… result Cross-model review (Codex) caught that severing every edge named react-native-fs, rather than only the ones a dependent declared optional, would delete the package even when another dependent hard-requires it — leaving that requirement dangling in the published shrinkwrap and breaking installs for every consumer. Reproduced with a two-parent fixture before fixing: the validator reported `other-pkg -> react-native-fs` unresolved. Only `optionalDependencies` edges are severed now, so a real requirement keeps the package alive for the whole tree. As a backstop the pruned result is scanned for required edges that resolve before the prune but not after, and the script throws rather than writing the file. Edges already unresolved in the input are ignored — a published shrinkwrap can legitimately carry some, and those are not ours to fail the build over. With the sever restricted to optional edges the backstop should be unreachable, since anything a surviving package requires is reachable without the severed edge. It stays because the cost of being wrong is every consumer's install. Also corrects the over-broad "untouchable by construction" claim in the script comment and dependencies.md. No change to the real tree: still 262 entries pruned, zero react-native entries left, zero unresolved required edges. Refs #1937 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cb1kenobi
reviewed
Jul 28, 2026
…une order Follow-ups from the cross-model review adjudication. The backstop is not unreachable as the previous comment claimed. Root-reachable packages cannot trip it, but entries the production walk never visits can — most usefully dev entries pointing into the react-native subtree, which is exactly what happens if this runs before the dev prune. Corrected the comment, added a test that pins the throw, and had the error point at prune-shrinkwrap-dev.mjs so the failure explains itself. That ordering was implicit in build.sh; noted it there so a future reorder does not produce a surprising build failure. Also added a fixture covering resolve()'s nested-first lookup and scoped names. Both occur in the real tree (@react-native/*) but every prior fixture was flat and unscoped, so that branch was only exercised incidentally. 12 unit tests. Real pipeline unchanged: 419 dev entries then 262 react-native entries pruned, zero unresolved required edges. Refs #1937 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… walk Review feedback from cb1kenobi. A prior commit added a nested/scoped fixture ten minutes after that review landed, which covers nested-first resolution, but not the case actually suggested: a react-native-fs whose only resolution path is a copy nested under its dependent, and a dependency of that copy reachable only by walking up two segments to the root. Both are now pinned. Verified the coverage is real rather than incidental by breaking the ancestor walk (clamping the loop to the deepest segment) and confirming 8 tests fail; restored, 13 pass. Real pipeline unchanged: 262 react-native entries pruned, zero unresolved required edges. Refs #1937 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 1, 2026
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.
Removes the react-native tree from what harper publishes. Part 1 of #1937.## What and why
alasqldeclaresoptionalDependencies: { "react-native-fs": "^2.20.0" }, andreact-native-fspeer-depends onreact-nativewithout marking it optional. npm 7+ auto-installs peer deps, so resolving alasql drags inreact-native,react,hermes,metroandreact-devtools-core— ~140MB, ~200 packages. None of it can execute: everyrequire('react-native-fs')inalasql/dist/alasql.fs.jssits behind anisReactNativeguard, and Harper only usesalasql.parseplus its function extensions. The published 5.1.23 shrinkwrap pins 794 packages, 64 of them react-native/hermes/metro.This runs alongsideprune-shrinkwrap-dev.mjs, which already enforces the same invariant — the published shrinkwrap should describe only the production tree a consumer installs (#1783).## Where to look**build-tools/prune-shrinkwrap-react-native.mjs** is the whole change; the rest is a build step, a doc entry, and tests.The reachability logic is what deserves scrutiny. It computes the set of packages reachable from the root with thereact-native-fsedge and without it, and deletes only the difference — so a package still reachable by any other path cannot be removed, and the removed set is derived rather than a hardcoded list of directory names that would silently rot as alasql's tree shifts.resolve()reimplements node_modules lookup (check<from>/node_modules/<name>, then each ancestor); that's the part most likely to be subtly wrong, though a mistake there fails safe — an unresolvable name simply means nothing gets marked reachable through it.On the current tree: 419 dev entries pruned, then 262 react-native entries, 1202 → 521. Zero unresolved required edges afterwards; the only new unresolved optional edge isalasql -> react-native-fs, the one being severed. (The published 5.1.23 shrinkwrap already carries 11 such unresolved optional/peer edges, so this is a pre-existing, normal pattern.)## Why notoverridesnpm honoursoverridesonly for the root project — an override in harper'spackage.jsondoes nothing for anyone installing harper. The published shrinkwrap, by contrast, is authoritative for registry installs.I verified that rather than assuming it, because the premise matters and one plausible failure mode would have sunk it: npm could have re-resolved the pruned optional dependency and pulled the subtree back. Serving a package with a pruned shrinkwrap from a local static registry (so npm takes the real packument +_hasShrinkwrapcode path, with the dependency still resolving from npmjs):shrinkwrap before prune: 331 entries, react-native present: yes shrinkwrap after prune: 23 entries, react-native present: no installed: 23 packages react-native tree in consumer tree: NO react-native-fs node: ABSENTConsequence: no stub package, no third-party empty module, and no change topackage.jsonor the committed lockfile.## Scope — what this does not fix- The Docker image.npm install <tarball>has no packument, so npm never learns the shrinkwrap exists and resolves the tree fresh frompackage.json. Confirmed on the same published 5.1.23 artifact: installed via the registry it honours the pin (fastify 5.8.5), installed from the tarball it resolves fresh (fastify 5.10.0, latest) and the react-native tree returns. The image is therefore also not reproducible — two builds of one commit weeks apart can ship different trees. Follow-up PR, which builds on this one; it has to land after this, since honouring an unpruned shrinkwrap would pin the react-native tree in.package-lock.jsonand are unaffected. Deliberately left alone: fixing it means either a stub package or a cryptic override, and it costs disk rather than shipped bytes.unitTests/buildTools/pruneShrinkwrapReactNative.test.js— 6 tests, the load-bearing one being that a package pulled in by react-native and depended on elsewhere survives. Note the siblingprune-shrinkwrap-dev.mjshas no tests; I added these because a wrong prune ships a broken shrinkwrap to every consumer.test:unit:main: 2795 passing, 2 failing —globalIsolationand a uWS 413 case. Both reproduce identically on pristineorigin/main(12/1 and 6/1 in isolation), and this diff only touchesbuild-tools/,unitTests/buildTools/anddependencies.md.## DocsNo companion documentation PR: this changes no public API, config option, schema directive, CLI surface or user-visible default — it only removes dead weight from what gets published. The behavioural note for future work lives inDESIGN.mdanddependencies.md.Closes Prune the react-native tree from the published shrinkwrap #1962🤖 Generated with Claude Code