Skip to content

fix(arborist): handle scripts set to null during rebuild#9185

Open
ap-b12 wants to merge 1 commit intonpm:latestfrom
ap-b12:fix/handle-null-scripts-rebuild
Open

fix(arborist): handle scripts set to null during rebuild#9185
ap-b12 wants to merge 1 commit intonpm:latestfrom
ap-b12:fix/handle-null-scripts-rebuild

Conversation

@ap-b12
Copy link
Copy Markdown

@ap-b12 ap-b12 commented Apr 2, 2026

Bug

Destructuring scripts when set to null throws a TypeError:

Cannot destructure property 'preinstall' of 'scripts' as it is null.

Problem

Even though there (seemingly) is a fallback in place when destructuring an object with scripts, this only works for undefined:

// `{ pkg: { scripts: undefined } }` --> `scripts` is `{}` (fallback)
// `{ pkg: { scripts: null } }` --> `scripts` is `null` (fallback does not apply)
const { gypfile, bin, scripts = {} } = pkg

When dealing with JSON, of course, undefined is not even a valid value in the first place, so null becomes the go-to alternative.

Fix

The solution is to handle scripts gracefully:

const scripts = null;

const { preinstall, install, postinstall, prepare } = scripts || {}; // Okay

console.log(preinstall); // undefined
console.log(install); // undefined
console.log(postinstall); // undefined
console.log(prepare); // undefined

Compared to:

const scripts = null;

const { preinstall, install, postinstall, prepare } = scripts; // Error

|| {} for scripts already exists in some places, but was missing from workspaces/arborist/lib/arborist/rebuild.js.

Note: At least in the following case, scripts = {} is still needed, as confirmed by current tests.

const { gypfile, bin, scripts = {} } = pkg

Test

A test was added to workspaces/arborist/test/arborist/rebuild.js.

[✓] Without the fix, the test fails.
[✓] After applying the fix, the test passes.

References

No existing issue could be found.

@ap-b12 ap-b12 requested a review from a team as a code owner April 2, 2026 22:30
@manzoorwanijk
Copy link
Copy Markdown
Contributor

I dug into whether "scripts": null is actually valid, and from what I can tell — it isn't, anywhere:

  • npm docs describe scripts as an "object hash" of commands. No mention of null as an accepted value. (source)
  • The SchemaStore JSON Schema (used by VS Code etc.) defines it as "type": "object" — not ["object", "null"], which is the pattern used elsewhere in the schema when null is intentionally allowed (e.g. in exports entries). (source)
  • npm pkg fix also doesn't catch or repair it. (Related: npm/package-json#81)

So the question I'd raise is: should we silently coerce null into {} (which arguably papers over invalid input), or should this be caught and warned about during normalization — the same way non-object scripts already triggers nonObjectScripts?

Not blocking — just wanted to flag this before we decide on the approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants