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

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

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

lolmaus
Copy link
Contributor

@lolmaus lolmaus commented Sep 14, 2023

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
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

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

❗ 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%) ⬆️

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

@lolmaus
Copy link
Contributor Author

lolmaus commented Sep 14, 2023

@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
Copy link
Contributor Author

lolmaus commented Sep 14, 2023

CC @mansona

@kategengler
Copy link
Member

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
Copy link
Contributor

NullVoxPopuli commented Sep 14, 2023

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
Copy link
Contributor

NullVoxPopuli commented Sep 19, 2023

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
Copy link
Contributor

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

@mansona
Copy link
Member

mansona commented Sep 26, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

lolmaus commented Sep 26, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

lolmaus commented Sep 26, 2023

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

@lolmaus lolmaus force-pushed the pnpm-resoultion-mode branch 3 times, most recently from 2e48b0b to db322b3 Compare September 26, 2023 13:51
@kategengler
Copy link
Member

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.

@lolmaus lolmaus force-pushed the pnpm-resoultion-mode branch 3 times, most recently from 5992170 to d861acf Compare September 27, 2023 09:11
Copy link
Member

@mansona mansona 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, and it matches what we discussed in the meeting yesterday (I think) 🎉

@lolmaus lolmaus changed the title Enforce resolution-mode=highest for pnpm versions 8.0.0 to 8.6.* ember-try: Enforce resolution-mode=highest for pnpm versions 8.0.0 to 8.6.* Oct 2, 2023
@ef4 ef4 merged commit 52d1b8e into ember-cli:master Oct 10, 2023
30 checks passed
@kategengler kategengler changed the title ember-try: Enforce resolution-mode=highest for pnpm versions 8.0.0 to 8.6.* Enforce resolution-mode=highest for pnpm versions 8.0.0 to 8.6.* Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants