fix(deps): declare tar as explicit runtime dependency#90
Merged
Conversation
`nodeup-npm/scripts/install.js` does `const tar =
require('tar')` at module load time. The pre-fix comment
("npm bundles tar; no extra dep") was incorrect: npm uses
tar internally, but it is not guaranteed to be reachable
from an installed package's script context. yarn/pnpm do
not hoist npm's internals; npm's modern flat-install
behavior is incidental.
Reproduced: `npm install` in a clean directory with no
declared dependency, then `node -e "require('tar')"` →
`Error: Cannot find module 'tar'`. yarn/pnpm users hit
this on every non-Windows postinstall, completely locking
them out of the npm install path that docs/installation.md
explicitly lists as supported.
Fix: add an explicit dependencies entry in
nodeup-npm/package.json, pinned to ^6.2.1 to match the
package's engines.node >= 14 floor (v7 requires Node 16+).
Replace the misleading comment with a real explanation
and a #63 cross-reference. Also drop the unused `zlib`
import the file was carrying (lint's `unused` static
check doesn't fire on .js files, so it slipped through).
Verified post-fix: `npm install` resolves tar; `node -e
"require('tar')"` returns `true`.
Closes #63
fad2ce6 to
7b4ef38
Compare
|
✔️ 7b4ef38 - Conventional commits check succeeded. |
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.
Summary
nodeup-npm/scripts/install.jsdoesconst tar = require('tar')at module load time, butnodeup-npm/package.jsonhad nodependenciesblock. The misleading comment claiming "npm bundles tar; no extra dep" was incorrect: npm uses tar internally, but it's not guaranteed to be reachable from an installed package's script context — yarn/pnpm do not hoist npm's internals, and npm's modern flat-install behavior is incidental.Reproduced live:
npm installin a clean dir with nodependencies, thennode -e "require('tar')"returnsCannot find module 'tar'. yarn/pnpm users hit this on every non-Windowspostinstall, completely locking them out of an install path thatdocs/installation.mdlists as supported.This PR declares
taras an explicit runtime dependency, pinned to^6.2.1to match the package'sengines.node >= 14floor (v7 requires Node 16+, a separate decision).Linked issues
Closes #63
Type of change
fix— bug fix (PATCH bump)feat— new feature (MINOR bump)refactor— no behavior changeperf— performance improvementdocs— documentation onlytest— tests onlychore— maintenance / dependency bumpci— CI/CD onlybuild— build system onlyChecklist
feat(scope): subject)make cilocally and it passes (tidy/fmt/vet/lint/test all green)npm install+node -e "require('tar')"round-trip in a clean tmp dir; not adding a JS test framework for a single require check — see "Notes" below)docs/, inline comments)Screenshots / output
Pre-fix reproduction on a fresh
npm install:Post-fix verification (with
"tar": "^6.2.1"independencies):Notes on testing
I deliberately did not introduce a JS test framework (jest, mocha, vitest, etc.) for what is ultimately a 1-line require-statement check — that would be a much larger change than the issue itself warrants, and it would add a Node-test-tooling-vs-version-pinning matrix to a project whose primary CI is
go test. The behavior change is small enough that the round-trip above is the most useful test; a future PR adding a JS-side smoke workflow is the right place for a structured JS test suite.The transitive
npm auditwarning that surfaces against unpinnedtar@^6.x(the node-tar GHSA advisories referenced in #65 — path traversal / symlink poisoning / hardlink escape) is intentionally deferred. Bumpingtarfrom v6 → v7 also raisesengines.nodefrom 14 → 16, which is a separate decision that belongs to #65.Files touched
nodeup-npm/package.json— adds:^6.2.1matchesengines.node: ">=14"(v7 requires Node 16+).nodeup-npm/scripts/install.js—// npm bundles tar; no extra depcomment with a 9-line explanation that names the failure mode (require fails at module-load time on yarn/pnpm), references bug(deps): nodeup-npm/install.js requires 'tar' with no declared dependency in package.json #63, and explains whytaris now a declared dependency.const zlib = require('zlib')import that the file was carrying (grep 'zlib\.' install.jshad no matches —lint unuseddoesn't fire on .js files so it slipped throughmake lint).CHANGELOG.md—### Fixedentry under[Unreleased]documenting the change with the bug(deps): nodeup-npm/install.js requires 'tar' with no declared dependency in package.json #63 reference and an explicit pointer that the v6→v7 / no-zip-slip side of this work is being deferred to bug(npm-wrapper): no zip-slip guard, Windows/ARM64 passes preinstall but 404s at download, partial state on failure #65.