Skip to content
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.

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

fix(pack, publish): default foreground-scripts to true #7158

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

ljharb
Copy link
Contributor

@ljharb ljharb commented Jan 18, 2024

Fixes #6816

This fixes a regression in both npm 9 and 10.

An alternative approach to adding a constructor could be, add something in BaseCommand that runs super, and then reads the default from the derived command class, and then sets the default - that way it'd be more declarative. Happy to change to something like that, if preferred.

@ljharb ljharb requested a review from a team as a code owner January 18, 2024 23:34
@ljharb ljharb changed the title fix(pack): default foreground-scripts to true fix(pack, publish): default foreground-scripts to true Jan 18, 2024
lib/commands/pack.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Check out the save config for how we are overriding the defaultDescription to describe subtleties like this.

@wraithgar
Copy link
Member

Let's put a pin in the base command update, since we'd want to include the other things for which the default is different (like save).

@wraithgar
Copy link
Member

Smoke tests will likely need snapshots updated.

@ljharb
Copy link
Contributor Author

ljharb commented Jan 19, 2024

How do I update smoke test snapshots?

@ljharb ljharb force-pushed the pack-foreground-scripts branch 2 times, most recently from cd1dcd8 to ec31fce Compare January 19, 2024 17:17
@wraithgar
Copy link
Member

npm run snap -w smoke-tests

@ljharb
Copy link
Contributor Author

ljharb commented Jan 19, 2024

I've updated the smoke test snapshots but they're still failing.

@lukekarrys
Copy link
Contributor

Smoke tests were failing because they made an assumption that the output of npm pack would only include the name of the resulting tarball which is no longer true with this change.

I fixed this in #7256. Once that lands, this PR can be rebased and should go green.

For reference here's what node . pack now looks like with this PR:

❯ node . pack 2> /dev/null

> npm@10.4.0 prepack
> node . run build -w docs


> @npmcli/docs@1.0.0 build
> node bin/build.js

Wrote 251 files
npm-10.4.0.tgz

@simonhaenisch
Copy link

simonhaenisch commented Aug 1, 2024

This change is problematic imo and it actually broke our tooling, because the stdout even with the --json flag now also contains the stdout of the pre/post hooks.

With --json i'd assume that you would be able to pipe the stdout and it only contains the command's json output, not some random output of lifecycle hooks.

(We were actually using the fact that it just used to print the tarball name to stdout, and I can see how that is maybe not the most robust idea since it's not a guaranteed API but with the --json flag I somehow expected it to work).

Edit: I just realized that --json doesn't print into a single line, so my initial idea of fixing it by piping through tail -n 1 doesn't work either. This change makes it very hard to separate the npm pack command's output with the one from the lifecycle hooks. Does adding sth like a --silent flag make sense? There's actually the (global?) --silent flag, and using it to prevent printing stdout of hooks would probably make sense to me.

Update: should have looked up before posting, there's already a bug linked there that was closed and mentions using --foreground-scripts=false which actually does what i need (but somehow isn't documented in the command's help).

@ljharb
Copy link
Contributor Author

ljharb commented Aug 1, 2024

@simonhaenisch the hooks are under your control - you can silence their output however you like unrelated to npm. You can also set --no-foreground-scripts if you want it to be false and restore the previous behavior, as you indicated.

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.

[BUG] prepack/postpack output is incorrectly suppressed
4 participants