Support lifecycle scripts for node #3259

thrix · 2024-07-17T19:36:34Z

The npm pack runs lifecycle scripts which can
spoil the standard output of the command and prevent
correct parsing of the created tarball.

For details see:

https://docs.npmjs.com/cli/v8/using-npm/scripts#life-cycle-scripts

Include a simple fix to parse out the tarball from the last line
and cover the fix with a simple test.

Signed-off-by: Miroslav Vadkerti mvadkert@redhat.com

The `npm pack` runs lifecycle scripts which can spoil the standard output of the command and prevent correct parsing of the created tarball. For details see: https://docs.npmjs.com/cli/v8/using-npm/scripts#life-cycle-scripts Include a simple fix to parse out the tarball from the last line and cover the fix with a simple test. Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>

thrix · 2024-07-17T20:04:09Z

The failed test fails for me also on main branch:

pre-commit on  main [?] is 📦 v3.7.1 via 🐍 v3.12.3 (venv) 
⬢ [environment:latest] ❯ pytest -k node
============================================================================================================ test session starts ============================================================================================================
platform linux -- Python 3.12.3, pytest-8.2.2, pluggy-1.5.0
rootdir: /var/home/mvadkert/git/github.com/pre-commit/pre-commit
configfile: tox.ini
plugins: env-1.1.3
collected 750 items / 739 deselected / 11 selected                                                                                                                                                                                          

tests/languages/node_test.py ........F..                                                                                                                                                                                              [100%]

================================================================================================================= FAILURES ==================================================================================================================
_____________________________________________________________________________________________________ test_node_hook_versions[default] ______________________________________________________________________________________________________

tmp_path = PosixPath('/tmp/pytest-of-mvadkert/pytest-9/test_node_hook_versions_defaul0'), version = 'default'

    @pytest.mark.parametrize('version', (C.DEFAULT, '18.14.0'))
    def test_node_hook_versions(tmp_path, version):
        _make_hello_world(tmp_path)
        ret = run_language(tmp_path, node, 'node-hello', version=version)
>       assert ret == (0, b'Hello World\n')
E       AssertionError: assert (1, b'Executa...o` not found') == (0, b'Hello World\n')
E         
E         At index 0 diff: 1 != 0
E         Use -v to get more diff

tests/languages/node_test.py:146: AssertionError
========================================================================================================== short test summary info ==========================================================================================================
FAILED tests/languages/node_test.py::test_node_hook_versions[default] - AssertionError: assert (1, b'Executa...o` not found') == (0, b'Hello World\n')
========================================================================================== 1 failed, 10 passed, 739 deselected in 84.34s (0:01:24) ==========================================================================================

asottile

the test you broke doesn't fail for me and didn't fail on main so you've got something to fix at least

please also start with an issue demonstrating the problem. I'm not convinced something is broken based on what you've written here and my experience with npm pack

asottile · 2024-07-17T20:28:52Z

tests/languages/node_test.py

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


rather than overload this function -- simply call it and then overwrite the output afterwards

ok, if you prefer it that way, will do once I resolve the breakage

thrix · 2024-07-17T20:51:49Z

the test you broke doesn't fail for me and didn't fail on main so you've got something to fix at least

please also start with an issue demonstrating the problem. I'm not convinced something is broken based on what you've written here and my experience with npm pack

Interesting, it failes on my system on main.
So maybe something to do with npm behaviour :( I am running latest Fedora 40 with

nodejs-20.12.2-1.fc40.x86_64
nodejs-npm-10.5.0-1.20.12.2.1.fc40.x86_64

Anyway, the problem was discovered while trying to add a pre-commit hook for https://github.com/RoadieHQ/backstage-entity-validator which uses these lifecycle hooks.

It can be easily reproduced this way:

❯ cat package.json 
{
    "name": "foo",
    "version": "0.0.1",
    "bin": {"foo": "./bin/main.js"},
    "scripts": {"prepare": "echo hello"},
    "dependencies": {"lodash": "*"}
}

❯ npm pack 2>/dev/null

> foo@0.0.1 prepare
> echo hello

hello
foo-0.0.1.tgz

The standard output is now spoiled with a few lines added by npm, at least this is what I see on the version I have.

thrix · 2024-07-17T20:52:27Z

I will try on ubuntu :)

thrix · 2024-07-17T21:30:40Z

I will try on ubuntu :)

Yeah, cannot reproduce the problem on ubuntu:latest container with

# npm --version
9.2.0
# node --version
v18.19.1

thrix · 2024-07-17T21:46:41Z

I will try on ubuntu :)

Yeah, cannot reproduce the problem on ubuntu:latest container with
# npm --version
9.2.0
# node --version
v18.19.1

ubuntu:devel (24.10) has also npm 9.2.0

So ubuntu will start hitting this when it gets npm 10.x :(

root@995732f4b4fc:/code# ~/node-v20.15.1-linux-x64/bin/npm pack

> foo@0.0.1 prepare
> echo hello

hello
npm notice
npm notice package: foo@0.0.1
npm notice Tarball Contents
npm notice 1.3kB foo-0.0.1.tgz
npm notice 162B package.json
npm notice Tarball Details
npm notice name: foo
npm notice version: 0.0.1
npm notice filename: foo-0.0.1.tgz
npm notice package size: 1.6 kB
npm notice unpacked size: 1.5 kB
npm notice shasum: cd37f4daec27573e554c92ba4258b629353075a8
npm notice integrity: sha512-U0AiHe2qcgkAj[...]m316HOS2V7zHg==
npm notice total files: 2
npm notice
foo-0.0.1.tgz

In any case, I expect if users start using new npm, more bug reports will come in from users using pre-commit hooks for node language.

asottile · 2024-07-17T22:10:12Z

hmm that feels like a pretty annoying regression in npm itself :S -- the output used to be on stdout in the past and was "fixed" in npm 7, seems like it's back again? is it every npm 10 or just the lastest fedora has packaged recently? (npm 10.x has been around for quite a while so I would have expected this sooner?)

thrix · 2024-07-18T17:01:54Z

hmm that feels like a pretty annoying regression in npm itself :S -- the output used to be on stdout in the past and was "fixed" in npm 7, seems like it's back again? is it every npm 10 or just the lastest fedora has packaged recently? (npm 10.x has been around for quite a while so I would have expected this sooner?)

So npm is bundled with nodejs according to the documentation, so I checked these versions 18.20.4 19.9.0 20.15.1 21.7.3 22.5.0 (some of them are EOL now) using nvm:

$ cat package.json
{
    "name": "foo",
    "version": "0.0.1",
    "bin": {"foo": "./bin/main.js"},
    "scripts": {"prepare": "echo hello"},
    "dependencies": {"lodash": "*"}
}

$ for version in 17.9.1 18.20.4 19.9.0 20.15.1 21.7.3 22.5.0; do
    nvm install $version &>/dev/null
    nvm exec $version true
    test $(nvm exec $version npm pack 2>/dev/null | wc -l) -eq 2 && echo [/] PASS || echo [x] FAIL
    echo
done

Running node v17.9.1 (npm v8.11.0)
[/] PASS

Running node v18.20.4 (npm v10.7.0)
[x] FAIL

Running node v19.9.0 (npm v9.6.3)
[/] PASS

Running node v20.15.1 (npm v10.7.0)
[x] FAIL

Running node v21.7.3 (npm v10.5.0)
[x] FAIL

Running node v22.5.0 (npm v10.8.2)
[x] FAIL

asottile · 2024-07-18T17:22:55Z

fwiw the other failure is likely the same as #3261 -- a bug in node

asottile · 2024-07-18T21:50:58Z

a better solution would be to use npm pack --json I believe

asottile · 2025-09-06T18:57:44Z

#3460 is a better approach to this -- but needs a less slow test

thrix force-pushed the npm-pack-output-parsing branch 2 times, most recently from c282e23 to e89705b Compare July 17, 2024 19:39

thrix force-pushed the npm-pack-output-parsing branch from e89705b to 1cee827 Compare July 17, 2024 20:21

thrix changed the title ~~node: support lifecycle scripts~~ Support lifecycle scripts for node Jul 17, 2024

asottile reviewed Jul 17, 2024

View reviewed changes

asottile mentioned this pull request May 9, 2025

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

Open

asottile closed this Sep 6, 2025

Uh oh!

Support lifecycle scripts for node #3259

Support lifecycle scripts for node #3259

Conversation

thrix commented Jul 17, 2024

Uh oh!

thrix commented Jul 17, 2024

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

asottile Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

thrix Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

thrix commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thrix commented Jul 17, 2024

Uh oh!

thrix commented Jul 17, 2024

Uh oh!

thrix commented Jul 17, 2024

Uh oh!

asottile commented Jul 17, 2024

Uh oh!

thrix commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Jul 18, 2024

Uh oh!

asottile commented Jul 18, 2024

Uh oh!

asottile commented Sep 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

thrix commented Jul 17, 2024 •

edited

Loading

thrix commented Jul 18, 2024 •

edited

Loading