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

Fixes issue with preversion script exposing the new updated package information instead of the currently existing one #51

Closed
wants to merge 2 commits into from

Conversation

luislobo
Copy link
Contributor

Fixes issue with preversion script exposing the new updated package information instead of the currently existing one

Addresses issue described in https://npm.community/t/npm-version-preversion-npm-package-version/1406

…nformation instead of the currently existing one
@luislobo luislobo requested a review from a team as a code owner August 15, 2018 16:49
@zkat zkat added the semver:major backwards-incompatible breaking changes label Aug 15, 2018
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you!

I believe this will be a semver-major change, so I'm gonna keep this PR open until npm@7 is ready to go out, and it should go in with that. Cheers!

@luislobo
Copy link
Contributor Author

@zkat Cool, thanks! In the meantime, I'm adding manual support in our preversion scripts, reading the package.json file, as the file do have the current version.

@luislobo
Copy link
Contributor Author

@zkat Don't forget about this one ;)

@luislobo
Copy link
Contributor Author

luislobo commented Mar 6, 2019

@zkat Friendly reminder about this PR

@chee
Copy link

chee commented Mar 6, 2019

@luislobo it's labeled as semver:major so they probably won't be looking at it until the next major version of npm is ready to go out, but they'll see it then! 😊

@luislobo
Copy link
Contributor Author

luislobo commented Sep 4, 2019

@isaacs Any news on getting this merged? We have had to workaround this issue reading the file ourselves to get the real information

@darcyclarke darcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
@JanithaR
Copy link

It's 2020 March already. Any particular reason why this isn't already merged?

@ljharb
Copy link
Contributor

ljharb commented Mar 16, 2020

Because it's a semver-major change, and npm 7 hasn't gone out yet?

@JanithaR
Copy link

Because it's a semver-major change, and npm 7 hasn't gone out yet?

I ask stupid questions. :) But man this should've been a patch.

@luislobo
Copy link
Contributor Author

luislobo commented May 8, 2020

@isaacs is this going to be included in npm 7?

@isaacs
Copy link
Contributor

isaacs commented May 15, 2020

So, this patch won't be in v7, because that section of code has been rewritten, and npm-lifecycle is being put to bed for the long sleep of deprecated modules.

But here's what the preversion environment looks like in v7, which I think satisfies the need that this is addressing:

$ cat package.json
{
  "name": "name",
  "version": "1.2.4",
  "scripts": {
    "preversion": "env | grep npm_"
  }
}

$ node ../cli version patch
> [email protected] preversion
> env | grep npm_
npm_config_metrics_registry=https://registry.npmjs.org/
npm_new_version=1.2.5
npm_old_version=1.2.4
npm_execpath=/Users/isaacs/dev/npm/cli/lib/npm.js
npm_config_access=public
npm_package_json=/Users/isaacs/dev/npm/foo/package.json
npm_command=version
npm_lifecycle_event=preversion
npm_config_node_gyp=/Users/isaacs/dev/npm/cli/node_modules/@npmcli/run-script/node_modules/node-gyp/bin/node-gyp.js
npm_lifecycle_script=env | grep npm_
npm_config_user_agent=npm/7.0.0-beta node/14.2.0 darwin x64
npm_node_execpath=/usr/local/bin/node
1.2.5

Let me know if there's more to do here, or if this meets your need. Thanks!

@luislobo
Copy link
Contributor Author

Looks good to me!

@luislobo luislobo closed this May 16, 2020
@luislobo luislobo deleted the fix-preversion-package-json branch May 16, 2020 02:03
antongolub pushed a commit to antongolub-forks/npm-cli that referenced this pull request May 18, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants