Skip to content

fix: Proper way of finding npm pack filename output. #3460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ovcharenko wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Proper way of finding npm pack filename output. #3460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions pre_commit/languages/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import contextlib
import functools
import json
import os
import sys
from collections.abc import Generator
Expand All @@ -17,6 +18,7 @@
from pre_commit.prefix import Prefix
from pre_commit.util import cmd_output
from pre_commit.util import cmd_output_b
from pre_commit.util import get_npm_version
from pre_commit.util import rmtree

ENVIRONMENT_DIR = 'node_env'
Expand Down Expand Up @@ -98,8 +100,33 @@ def install_environment(
)
lang_base.setup_cmd(prefix, local_install_cmd)

_, pkg, _ = cmd_output('npm', 'pack', cwd=prefix.prefix_dir)
pkg = prefix.path(pkg.strip())
npm_version = get_npm_version()

args = ['npm', 'pack', '--json']
# https://docs.npmjs.com/cli/v11/using-npm/changelog#1100-pre0-2024-11-26
if npm_version >= (11, 0, 0):
args.append('--ignore-scripts')
Comment on lines +106 to +108
Copy link
Member

@asottile asottile Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems unrelated to your patch / not necessary


_, pkg, _ = cmd_output(*args, cwd=prefix.prefix_dir)
try:
pkg_json = json.loads(pkg)
except json.JSONDecodeError as e:
raise ValueError('Failed to parse npm pack output as JSON.') from e
Comment on lines +113 to +114
Copy link
Member

@asottile asottile Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just let it raise no reason to catch and reraise


if not pkg_json:
raise ValueError('JSON array from npm pack is empty.')

if not isinstance(pkg_json, list):
raise ValueError('Expected npm pack output to be a JSON array.')

filename = pkg_json[0].get('filename')
if filename is None:
raise KeyError(
"Key 'filename' not found in the first element "
'of the JSON array.',
)
Comment on lines +116 to +127
Copy link
Member

@asottile asottile Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just let these raise


pkg = prefix.path(filename)

install = ('npm', 'install', '-g', pkg, *additional_dependencies)
lang_base.setup_cmd(prefix, install)
Expand Down
11 changes: 11 additions & 0 deletions pre_commit/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import errno
import importlib.resources
import os.path
import re
import shutil
import stat
import subprocess
Expand Down Expand Up @@ -237,3 +238,13 @@ def rmtree(path: str) -> None:

def win_exe(s: str) -> str:
return s if sys.platform != 'win32' else f'{s}.exe'


def get_npm_version() -> tuple[int, ...]:
_, out, _ = cmd_output('npm', '--version')
version_match = re.match(r'^(\d+)\.(\d+)\.(\d+)', out)

if version_match is None:
return 0, 0, 0
else:
return tuple(map(int, version_match.groups()))
Comment on lines +241 to +250
Copy link
Member

@asottile asottile Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new code should not end up in utils -- also this is only used in one place

18 changes: 16 additions & 2 deletions tests/languages/node_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ def test_installs_without_links_outside_env(tmpdir):
assert cmd_output('foo')[1] == 'success!\n'


def _make_hello_world(tmp_path):
package_json = '''\
def _make_hello_world(tmp_path, package_json=None):
package_json = package_json or '''\
Comment on lines +116 to +117
Copy link
Member

@asottile asottile Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than changing this function -- call it and then write a package.json in the specific test below

{"name": "t", "version": "0.0.1", "bin": {"node-hello": "./bin/main.js"}}
'''
tmp_path.joinpath('package.json').write_text(package_json)
Expand All @@ -132,6 +132,20 @@ def test_node_hook_system(tmp_path):
assert ret == (0, b'Hello World\n')


def test_node_with_prepare_script(tmp_path):
Copy link
Member

@asottile asottile Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you show the output of this failing before your patch?

package_json = '''
{
"name": "t",
"version": "0.0.1",
"bin": {"node-hello": "./bin/main.js"},
"scripts": {"prepare": "echo prepare"}
}
'''
_make_hello_world(tmp_path, package_json)
ret = run_language(tmp_path, node, 'node-hello')
assert ret == (0, b'Hello World\n')


def test_node_with_user_config_set(tmp_path):
cfg = tmp_path.joinpath('cfg')
cfg.write_text('cache=/dne\n')
Expand Down
Loading