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

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
3 participants