-
Notifications
You must be signed in to change notification settings - Fork 1k
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
build(deps): bump PNpm from 8.6.12 to 8.7.6 #7899
Conversation
@@ -3978,7 +3978,7 @@ | |||
it "updates the manifest and lockfile" do | |||
expect(updated_files.map(&:name)).to match_array(%w(package.json pnpm-lock.yaml)) | |||
|
|||
expect(updated_pnpm_lock.content).to include("/node-adodb@5.0.2:") | |||
expect(updated_pnpm_lock.content).to include("/node-adodb@5.0.3:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the result of the following change :
The default value of the resolution-mode setting is changed to highest. This setting was changed to lowest-direct in v8.0.0 and some users were not happy with the change. A twitter poll concluded that most of the users want the old behaviour (resolution-mode set to highest by default). This is a semi-breaking change but should not affect users that commit their lockfile #6463.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deivid-rodriguez Your input would be appreciated here 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_deivid-rodriguez just headed out on holiday and will be offline for a few weeks (I think he might be 🚴 ?)
But I agree it'd be good to get his input on this, so if no one else on the team picks this up and you don't hear from him by Sept 20th, feel free to ping him again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fine @yeikel, but we may want to make the test resilient to new releases of this dependency. Maybe by using an exact dependency? This test claims to be dealing with incorrect platform dependencies, so I guess such as change would not affect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the feedback that they received, I do not expect this to change again anytime soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deivid-rodriguez What do you suggest we do now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean you don't expect upstream PNPM to change default resolution mode again soon, or you don't expect node-adodb
to release 5.0.4 anytime soon?
My suggestion was aimed at making this spec resilient to a potential release of node-adodb 5.0.4.
d525c70
to
82d69c9
Compare
29fe388
to
fb5d3c6
Compare
3f5fb62
to
8dd57dc
Compare
2c20b86
to
bf34d59
Compare
4edec5f
to
4c0aad6
Compare
bf741a4
to
a2744c9
Compare
Release notes: https://github.com/pnpm/pnpm/releases/tag/v8.7.0 https://github.com/pnpm/pnpm/releases/tag/v8.7.1 https://github.com/pnpm/pnpm/releases/tag/v8.7.2 https://github.com/pnpm/pnpm/releases/tag/v8.7.3 https://github.com/pnpm/pnpm/releases/tag/v8.7.4 https://github.com/pnpm/pnpm/releases/tag/v8.7.5 https://github.com/pnpm/pnpm/releases/tag/v8.7.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this. We can add the improvement to the specs that I mentioned if it ends up biting us again.
Thanks @yeikel! |
I was about to work on the tests you recommended. Sorry about the delay I'll send that in a follow up PR sometime this week |
No problem, thanks as always for your contributions and patience! |
Release notes:
https://github.com/pnpm/pnpm/releases/tag/v8.7.0
https://github.com/pnpm/pnpm/releases/tag/v8.7.1
https://github.com/pnpm/pnpm/releases/tag/v8.7.2
https://github.com/pnpm/pnpm/releases/tag/v8.7.3
https://github.com/pnpm/pnpm/releases/tag/v8.7.4
https://github.com/pnpm/pnpm/releases/tag/v8.7.5
https://github.com/pnpm/pnpm/releases/tag/v8.7.6