-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Enforce resolution-mode=highest for pnpm versions 8.0.0 to 8.6.* #966
Conversation
ea5153e
to
2090108
Compare
Codecov Report
@@ Coverage Diff @@
## master #966 +/- ##
==========================================
+ Coverage 94.59% 94.73% +0.14%
==========================================
Files 18 18
Lines 518 532 +14
==========================================
+ Hits 490 504 +14
Misses 28 28
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
2090108
to
188b123
Compare
@bertdeblock Mind having a look? I did my best to use the Backup API correctly. But I needed to make a couple of small updates into the Backup util. |
CC @mansona |
I understand the motivation but this seems a bit overkill. What if we just warn or error on the affected pnpm versions? |
I would prefer warn/error as well. Folks using @lolmaus , |
idea?: when in the "danger range", check: $`npm config get resolution-mode` === 'highest' and if that is false, throw the error/warn Note, when not set: ❯ npm config get resolution-mode
undefined |
Personally, I would do nothing. I don't feel it's ember-try's responsibility to warn about, or error on this. |
I strongly disagree on this one. If we do nothing in this case then the default Just to be explicit (I'm not trying to tell you anything you already know) but |
I understand what it means to use |
Then they may receive incorrect test results with For |
Maybe, but |
86328ea
to
5e90bc7
Compare
@bertdeblock We're gonna have an Ember Tooling Team meeting today at 17:00 CEST at Ember Discord. I'm gonna bring this matter up again per your objection. Please join and contribute to the discussion. 🙏 I'm |
2e48b0b
to
db322b3
Compare
I think we could get pretty far by noting the version issue / resolution configuration in the README and publicizing it. For a fix to be protective going forward users would need to update ember-try so this won't fix anything for those leaving their addons alone for awhile. Support for @bertdeblock's point that users may choose that type of resolution is a very good one. Our README should warn that if that mode is chosen, they may be losing the benefits of ember-try unless they explicitly have scenarios for each version. |
5992170
to
d861acf
Compare
d861acf
to
cb5f1a1
Compare
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.
Looks good to me, and it matches what we discussed in the meeting yesterday (I think) 🎉
pnpm versions 8.0.0 to 8.6.* had the
resolution-mode
setting inverted which caused troubles with unexpected versions of dependencies pulled forember-try
tests.This is an attempt to fix those issues by enfrocing
resolution-mode=highest
via project-local.npmrc
.