Enforce resolution-mode=highest for pnpm versions 8.0.0 to 8.6.*

[lolmaus]

pnpm versions 8.0.0 to 8.6.* had the resolution-mode setting inverted which caused troubles with unexpected versions of dependencies pulled for ember-try tests.

This is an attempt to fix those issues by enfrocing resolution-mode=highest via project-local .npmrc.

[codecov[bot]]

Codecov Report

Merging #966 (2d89e5c) into master (2438dca) will increase coverage by 0.14%. The diff coverage is 100.00%.

:exclamation: Current head 2d89e5c differs from pull request most recent head cb5f1a1. Consider uploading reports for the commit cb5f1a1 to get more accurate results

@@            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              

Files	Coverage Δ
lib/dependency-manager-adapters/pnpm.js	92.20% <100.00%> (+1.73%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[lolmaus]

@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.

[lolmaus]

CC @mansona

[kategengler]

I understand the motivation but this seems a bit overkill. What if we just warn or error on the affected pnpm versions?

cc @NullVoxPopuli

[NullVoxPopuli]

I would prefer warn/error as well.

Folks using pnpm should (hopefully) already be used to frequent updates and be striving for being on the latest version.

@lolmaus , I do appreciate all your work tho, it looks like a lot of thought and time went in to this <3

[NullVoxPopuli]

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

[bertdeblock]

Personally, I would do nothing. I don't feel it's ember-try's responsibility to warn about, or error on this.

[mansona]

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 embroiderSafe() and embroiderOptimised() ember-try scenarios on addons will be broken without people knowing that they are broken.

Just to be explicit (I'm not trying to tell you anything you already know) but embroiderSafe() from @embroider/test-setup currently adds a ^3.0.1 version dependency on @embroider/core @embroider/compat and @embroider/webpack and that means that with resolution-mode=lowest-direct you will not be testing your addons against the latest Embroider. This is just one example where the expected behaviour is just wrong and people need to be informed if ember-try is doing something that isn't what is expected.

[bertdeblock]

I understand what it means to use lowest-direct. I'm just failing to see why ember-try should error on this. Seems like the user's responsibility, as they choose to use pnpm v8. What if the user wants to use lowest-direct (either implicit or explicitly defined)?

[lolmaus]

What if the user wants to use lowest-direct (either implicit or explicitly defined)?

Then they may receive incorrect test results with ember-try?

For ember-try tests to be consistent, the resolution-mode must be identical for an addon developer, addon consumer and CI.

[bertdeblock]

Maybe, but lowest-direct is a valid resolution mode and if a user chooses to (explicitly) use that mode, I feel ember-try should respect that.

[lolmaus]

@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 lolmaus at Discord.

[kategengler]

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 embroiderSafe() and embroiderOptimised() ember-try scenarios on addons will be broken without people knowing that they are broken.

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 pnpm is relatively new and those using it before official support landed in ember-cli (5.3) are more likely to be the type of user that is aware of this problem.

@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.