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 package id in shrinkwrap lifecycle step output #288

Closed
wants to merge 3 commits into from

Conversation

bz2
Copy link
Contributor

@bz2 bz2 commented Nov 6, 2019

Currently all logging related to shrinkwrap steps reports 'undefined'
for the package in output and log messages.

This is due to the package associated with the idealTree being
recreated in the savePackageJson() method which precedes these
steps. For now, just copy forward the _id attribute which lifecycle
logging expects, but note that mutating package here is surprising.

Fixes npm/npm#20756

@bz2 bz2 requested a review from a team as a code owner November 6, 2019 20:12
Currently all logging related to shrinkwrap steps reports 'undefined'
for the package in output and log messages.

This is due to the package associated with the `idealTree` being
recreated in the `savePackageJson()` method which precedes these
steps. For now, just copy forward the `_id` attribute which lifecycle
logging expects, but note that mutating `package` here is surprising.

Fixes npm/npm#20756
Not supported on node 6 apparently.

Can get the same effect by explictly matching the newline character.
@claudiahdz claudiahdz added the Needs Discussion is pending a discussion label Nov 12, 2019
@bz2
Copy link
Contributor Author

bz2 commented May 23, 2020

@ruyadorno @claudiahdz Is there anything I can do to help get this reviewed and landed?

What kind of discussion is needed exactly? This is a pretty simple change that improves error output for steps after post install.

@isaacs
Copy link
Contributor

isaacs commented May 26, 2020

This looks good to me. It's irrelevant in npm v7, but we may as well fix this little thing in the meantime.

Sorry for the delay, we're all pretty focused on getting the next major done. Thanks for your patience.

@bz2
Copy link
Contributor Author

bz2 commented Jun 8, 2020

@claudiahdz It it possible for you to merge this? I do not have write access to actually land.

@isaacs Thank you for the review. I like the streamlined upcoming changes, though it now doesn't say which script has failed:

$ node ~/branches/npm-cli/lib/npm.js i --save-exact
npm ERR! code 1
npm ERR! path /tmp/pkg/node_modules/fail
npm ERR! command failed
npm ERR! command sh -c false

@ruyadorno ruyadorno added Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion labels Jun 11, 2020
@ruyadorno ruyadorno added this to the OSS - Sprint 8 milestone Jun 11, 2020
@claudiahdz
Copy link
Contributor

This will be added on 6.14.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined preshrinkwrap, shrinkwrap and postshrinkwrap
4 participants