cli
cli copied to clipboard
npm
fix(libnpmpack): run scripts in foreground when foregroundScripts === true
Before this change, npm pack would not respect --foreground-scripts, so when {pre,post}pack scripts were run, the output would be polluted with output from those scripts.
I'm not sure how I'd test the output of this from libnpmpack tests, like the arborist test does. If anyone can give me any pointers on that, I'd be happy to add a test.
Also: tips on decreasing the length of the commit message are appreciated, the cutoff is a bit jarring.
References
Fixes #4121.
This is failing due to the reduced test coverage. I can't figure out a way to test this behavior just from using libnpmpack -- I'd have to either spawn a new process or mock spawn.
What's the best way to go about this?
This is failing due to the reduced test coverage. I can't figure out a way to test this behavior just from using
libnpmpack-- I'd have to either spawn a new process or mockspawn.What's the best way to go about this?
While we haven't started using it in this repo, mocking spawn is a thing we do in other npm cli subdependencies, and in the npm tests too. We use https://github.com/wraithgar/spawk. For good patterns you can check out the start stop and test tests in the npm cli itself.
tips on decreasing the length of the commit message are appreciated, the cutoff is a bit jarring.
fix: obey foregroundScript would probably suffice. The commit will only end up in the libnpmpack changelog so you don't really need to add that to the scope.
@wraithgar I attempted to use spawk here, but am running into an issue. The command and arguments that are passed to spawn line up with what I'm mocking, but it still says that it wasn't called. Am I missing something obvious here?
It looks like overriding child_process.spawn isn't working here, but I don't know why it would work in the other tests but not this one. (Maybe the package boundary?)
Hey @wraithgar, just bumping this in case you forgot about my last comment :)
I did see it. We are currently focused on breaking changes for npm 9 and there isn't any bandwidth right now for anyone to help you on this. If you can figure the tests out, great. If not, this can wait a bit till someone can look at it.
I figured it out -- spawk needed to be initialized before everything else was imported. (Technically this would work all the same if I moved it before just a subset of the imports, but I figured this is best.)
I guess I have to update package-lock.json. Do I just run npm ci to do that properly (so I don't update any other versions in the lockfile), or is there some other way to do it?
How did you add spawk? If you used npm install it should have updated the lockfile accordingly.
npm install -D spawk -ws libnpmpack
After reverting workspaces/libnpmpack/package.json and DEPENDENCIES.md to their state before this commit, and running this command, I get these errors.
Well fiddlesticks. Good news is you found a case that causes that error to happen so we can debug it now. Bad news is you can't install and update the md file. Go ahead and install with --ignore-scripts which will prevent that script from trying to update the md file. We may have to fix it to get CI to go green but at least you can get everything else about the PR ready to go.
Can you also make sure your branch is rebased against latest? It's possible that some things were moved around since you started. Lots of things have been shuffling as part of npm 9.
@winterqt I am so sorry I told you to run the wrong command. It should be -w not -ws.
I feel really bad about this. You've worked really hard on this PR and I know it's probably frustrating that this is the part we're getting blocked on. The PR itself looks great and once we sort the problem of adding spawk out this will land.
In the interest of getting this out today (release day!) I went ahead and fixed the spawk install in another branch, leaving your work in its own commit: https://github.com/npm/cli/pull/5645