fix: Proper way of finding `npm pack` `filename` output. by ovcharenko · Pull Request #3460 · pre-commit/pre-commit

ovcharenko · 2025-05-09T10:50:27Z

That will fix pollution of installation path with TypeScript output:

E           pre_commit.util.CalledProcessError: command: ('/private/var/folders/sd/qk6llt717b99lp1wgtym5qwm0000gn/T/pytest-of-local.user/pytest-16/test_node_typescript_hook_syst0/node_env-system/bin/node', '/opt/homebrew/bin/npm', 'install', '-g', '/private/var/folders/sd/qk6llt717b99lp1wgtym5qwm0000gn/T/pytest-of-local.user/pytest-16/test_node_typescript_hook_syst0/> t@0.0.1 prepare\n> tsc\n\nt-0.0.1.tgz')
E           return code: 254
E           stdout: (none)
E           stderr:
E               npm warn tarball tarball data for file:/private/var/folders/sd/qk6llt717b99lp1wgtym5qwm0000gn/T/pytest-of-local.user/pytest-16/test_node_typescript_hook_syst0/> t@0.0.1 prepare> tsct-0.0.1.tgz (null) seems to be corrupted. Trying again.
E               npm warn tarball tarball data for file:/private/var/folders/sd/qk6llt717b99lp1wgtym5qwm0000gn/T/pytest-of-local.user/pytest-16/test_node_typescript_hook_syst0/> t@0.0.1 prepare> tsct-0.0.1.tgz (null) seems to be corrupted. Trying again.
E               npm error code ENOENT
E               npm error syscall open
E               npm error path /private/var/folders/sd/qk6llt717b99lp1wgtym5qwm0000gn/T/pytest-of-local.user/pytest-16/test_node_typescript_hook_syst0/> t@0.0.1 prepare> tsct-0.0.1.tgz
E               npm error errno -2
E               npm error enoent ENOENT: no such file or directory, open '/private/var/folders/sd/qk6llt717b99lp1wgtym5qwm0000gn/T/pytest-of-local.user/pytest-16/test_node_typescript_hook_syst0/> t@0.0.1 prepare> tsct-0.0.1.tgz'
E               npm error enoent This is related to npm not being able to find a file.
E               npm error enoent
E               npm error A complete log of this run can be found in: /Users/local.user/.npm/_logs/2025-05-09T10_21_58_861Z-debug-0.log

asottile · 2025-05-09T14:07:23Z

the other branch which tried to solve this is here and my recommendation: #3259 (comment)

ovcharenko · 2025-05-09T14:52:06Z

the other branch which tried to solve this is here and my recommendation: #3259 (comment)

That doesn't help much IMHO, because the JSON is part of other noisy output. So instead of one-line fix you will have to find JSON object and handle all error parsing.

asottile · 2025-05-09T14:53:35Z

pretty sure in json mode it does a better job of obscuring the nonsense

ovcharenko · 2025-05-09T15:02:41Z

pretty sure in json mode it does a better job of obscuring the nonsense

19d0b42 should do this, right?

ovcharenko · 2025-05-12T15:37:00Z

@asottile any chance to get this merged any soon?

asottile · 2025-05-23T20:39:23Z

tests/languages/node_test.py

+  "devDependencies": {
+    "typescript": "^5.3.0"
+  }


this is going to make the test prohibitively slow -- can you make a faster example?

@asottile I took an example from #3259 instead. Will that work?

pre_commit/languages/node.py

asottile · 2025-10-02T17:46:55Z

pre_commit/util.py

+
+
+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()))


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

asottile · 2025-10-02T17:47:09Z

pre_commit/languages/node.py

+        # https://docs.npmjs.com/cli/v11/using-npm/changelog#1100-pre0-2024-11-26
+        if npm_version >= (11, 0, 0):
+            args.append('--ignore-scripts')


this seems unrelated to your patch / not necessary

asottile · 2025-10-02T17:47:19Z

pre_commit/languages/node.py

+        except json.JSONDecodeError as e:
+            raise ValueError('Failed to parse npm pack output as JSON.') from e


just let it raise no reason to catch and reraise

asottile · 2025-10-02T17:47:55Z

pre_commit/languages/node.py

+        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.',
+            )


I would just let these raise

asottile · 2025-10-02T17:48:14Z

tests/languages/node_test.py

+def _make_hello_world(tmp_path, package_json=None):
+    package_json = package_json or '''\


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

asottile · 2025-10-02T17:48:36Z

tests/languages/node_test.py

    assert ret == (0, b'Hello World\n')


+def test_node_with_prepare_script(tmp_path):


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

ovcharenko mentioned this pull request May 9, 2025

feat: Add pre-commit hooks. npalm/action-docs#830

Open

ovcharenko changed the title ~~fix: Process only the last line of npm pack output.~~ fix: Proper way of finding npm pack filename output. May 9, 2025

asottile reviewed May 23, 2025

View reviewed changes

asottile mentioned this pull request Sep 6, 2025

Support lifecycle scripts for node #3259

Closed

ovcharenko force-pushed the main branch 4 times, most recently from bbabec2 to 928edc0 Compare September 26, 2025 15:51

fix: Use JSON to parse npm pack output.

24d595f

ovcharenko force-pushed the main branch from 404ec8b to 24d595f Compare September 26, 2025 15:57

asottile reviewed Oct 2, 2025

View reviewed changes

		except json.JSONDecodeError as e:
		raise ValueError('Failed to parse npm pack output as JSON.') from e

		def _make_hello_world(tmp_path, package_json=None):
		package_json = package_json or '''\

		assert ret == (0, b'Hello World\n')


		def test_node_with_prepare_script(tmp_path):

Uh oh!

Conversation

ovcharenko commented May 9, 2025

Uh oh!

asottile commented May 9, 2025

Uh oh!

ovcharenko commented May 9, 2025

Uh oh!

asottile commented May 9, 2025

Uh oh!

ovcharenko commented May 9, 2025

Uh oh!

ovcharenko commented May 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants