Skip to content

fix(nm): optimize hoisting by treating peer deps same as other deps #6517

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 24, 2024
Merged

fix(nm): optimize hoisting by treating peer deps same as other deps #6517

merged 3 commits into from
Sep 24, 2024

Conversation

akwodkiewicz
Copy link
Contributor

@akwodkiewicz akwodkiewicz commented Sep 24, 2024 •
edited
Loading

What's the problem this PR addresses?

Fixes #6516

How did you fix it?

Simplified the sorting algorithm in getHoistIndetMap.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

akwodkiewicz and others added 2 commits September 24, 2024 12:23
@akwodkiewicz
test(nm): peer deps not having higher precedence when hoisting
850f116
@akwodkiewicz @larixer
fix(nm): sum peer and direct usages when calculating hoisting prefere…
af874a5 
…nces

Until now the peerDependencies had a higher preference that could lead to
unoptimal hoisting behaviour.

Thanks to @larixer for pointing out to this particular snippet.

Co-Authored-By: Victor Vlasenko <[email protected]>
@akwodkiewicz akwodkiewicz changed the title Fix/nm hoisting algorithm fix(nm): optimize hoisting by treating peer deps same as other deps Sep 24, 2024
@larixer larixer added this pull request to the merge queue Sep 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 24, 2024
@larixer larixer added this pull request to the merge queue Sep 24, 2024
Merged via the queue into yarnpkg:master with commit d63d411 Sep 24, 2024
32 checks passed
@ChALkeR ChALkeR mentioned this pull request Nov 29, 2024
Merged
@jeffwidman jeffwidman mentioned this pull request Dec 13, 2024
Merged
jeffwidman added a commit to dependabot/dependabot-core that referenced this pull request Dec 13, 2024
@jeffwidman
Update test to match new handling of peer deps
6aceb80 
This is the failing test: 

https://github.com/dependabot/dependabot-core/blob/8f037cf1be97f2a0c1f383d74479ebe2a48e0c17/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb#L855-L875

Full commit including lockfiles: 1dcda58

Interestingly, it doesn't fail in Yarn `4.3.1` as seen in:
* #8265

Poking through the changelog between 4.3.1 and 4.5.3, this seems to be relevant upstream PR:
* yarnpkg/berry#6517

My understanding of peer dependency handling in Yarn is hazy at best, but after reading the PR description, it _looks_ like the algorithm changed and now the peer dependency can be updated and not necessarily held back. 

That would explain [this test failure](https://github.com/dependabot/dependabot-core/actions/runs/12307737164/job/34351931150?pr=11123#step:5:56):
```
  1) Dependabot::NpmAndYarn::UpdateChecker::VersionResolver#latest_resolvable_version with a yarn berry lockfile when updating a dependency with a peer requirement is expected to eq #<Gem::Version "15.2.0">
     Failure/Error: it { is_expected.to eq(Gem::Version.new("15.2.0")) }

       expected: #<Gem::Version "15.2.0">
            got: #<Gem::Version "16.3.1">

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -Gem::Version.new("15.2.0")
       +Gem::Version.new("16.3.1")
     # ./spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb:873:in `block (5 levels) in <top (required)>'
     # /home/dependabot/common/spec/spec_helper.rb:66:in `block (2 levels) in <top (required)>'
     # /home/dependabot/dependabot-updater/vendor/ruby/3.3.0/gems/webmock-3.24.0/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
```
jeffwidman added a commit to dependabot/dependabot-core that referenced this pull request Dec 13, 2024
@jeffwidman
Update test to match new handling of peer deps
bec32f4 
This is the failing test: 

https://github.com/dependabot/dependabot-core/blob/8f037cf1be97f2a0c1f383d74479ebe2a48e0c17/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb#L855-L875

Full commit including lockfiles: 1dcda58

Interestingly, it doesn't fail in Yarn `4.3.1` as seen in:
* #8265

Poking through the changelog between 4.3.1 and 4.5.3, this seems to be relevant upstream PR:
* yarnpkg/berry#6517

My understanding of peer dependency handling in Yarn is hazy at best, but after reading the PR description, it _looks_ like the algorithm changed and now the peer dependency can be updated and not necessarily held back. 

That would explain [this test failure](https://github.com/dependabot/dependabot-core/actions/runs/12307737164/job/34351931150?pr=11123#step:5:56):
```
  1) Dependabot::NpmAndYarn::UpdateChecker::VersionResolver#latest_resolvable_version with a yarn berry lockfile when updating a dependency with a peer requirement is expected to eq #<Gem::Version "15.2.0">
     Failure/Error: it { is_expected.to eq(Gem::Version.new("15.2.0")) }

       expected: #<Gem::Version "15.2.0">
            got: #<Gem::Version "16.3.1">

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -Gem::Version.new("15.2.0")
       +Gem::Version.new("16.3.1")
     # ./spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb:873:in `block (5 levels) in <top (required)>'
     # /home/dependabot/common/spec/spec_helper.rb:66:in `block (2 levels) in <top (required)>'
     # /home/dependabot/dependabot-updater/vendor/ruby/3.3.0/gems/webmock-3.24.0/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
```
jeffwidman added a commit to dependabot/dependabot-core that referenced this pull request Dec 13, 2024
@jeffwidman
Update test to match new handling of peer deps
10155e8 
This is the failing test: 

https://github.com/dependabot/dependabot-core/blob/8f037cf1be97f2a0c1f383d74479ebe2a48e0c17/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb#L855-L875

Full commit including lockfiles: 1dcda58

Interestingly, it doesn't fail in Yarn `4.3.1` as seen in:
* #8265

Poking through the changelog between 4.3.1 and 4.5.3, this seems to be relevant upstream PR:
* yarnpkg/berry#6517

My understanding of peer dependency handling in Yarn is hazy at best, but after reading the PR description, it _looks_ like the algorithm changed and now the peer dependency can be updated and not necessarily held back. 

That would explain [this test failure](https://github.com/dependabot/dependabot-core/actions/runs/12307737164/job/34351931150?pr=11123#step:5:56):
```
  1) Dependabot::NpmAndYarn::UpdateChecker::VersionResolver#latest_resolvable_version with a yarn berry lockfile when updating a dependency with a peer requirement is expected to eq #<Gem::Version "15.2.0">
     Failure/Error: it { is_expected.to eq(Gem::Version.new("15.2.0")) }

       expected: #<Gem::Version "15.2.0">
            got: #<Gem::Version "16.3.1">

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -Gem::Version.new("15.2.0")
       +Gem::Version.new("16.3.1")
     # ./spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb:873:in `block (5 levels) in <top (required)>'
     # /home/dependabot/common/spec/spec_helper.rb:66:in `block (2 levels) in <top (required)>'
     # /home/dependabot/dependabot-updater/vendor/ruby/3.3.0/gems/webmock-3.24.0/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
```
jeffwidman added a commit to dependabot/dependabot-core that referenced this pull request Dec 13, 2024
@jeffwidman
Bump yarn to 4.5.3 (#11123)
21cb492 
* Bump yarn to 4.5.3

* Update test to match new handling of peer deps

This is the failing test: 

https://github.com/dependabot/dependabot-core/blob/8f037cf1be97f2a0c1f383d74479ebe2a48e0c17/npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb#L855-L875

Full commit including lockfiles: 1dcda58

Interestingly, it doesn't fail in Yarn `4.3.1` as seen in:
* #8265

Poking through the changelog between 4.3.1 and 4.5.3, this seems to be relevant upstream PR:
* yarnpkg/berry#6517

My understanding of peer dependency handling in Yarn is hazy at best, but after reading the PR description, it _looks_ like the algorithm changed and now the peer dependency can be updated and not necessarily held back. 

That would explain [this test failure](https://github.com/dependabot/dependabot-core/actions/runs/12307737164/job/34351931150?pr=11123#step:5:56):
```
  1) Dependabot::NpmAndYarn::UpdateChecker::VersionResolver#latest_resolvable_version with a yarn berry lockfile when updating a dependency with a peer requirement is expected to eq #<Gem::Version "15.2.0">
     Failure/Error: it { is_expected.to eq(Gem::Version.new("15.2.0")) }

       expected: #<Gem::Version "15.2.0">
            got: #<Gem::Version "16.3.1">

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -Gem::Version.new("15.2.0")
       +Gem::Version.new("16.3.1")
     # ./spec/dependabot/npm_and_yarn/update_checker/version_resolver_spec.rb:873:in `block (5 levels) in <top (required)>'
     # /home/dependabot/common/spec/spec_helper.rb:66:in `block (2 levels) in <top (required)>'
     # /home/dependabot/dependabot-updater/vendor/ruby/3.3.0/gems/webmock-3.24.0/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larixer larixer larixer approved these changes

@merceyz merceyz merceyz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug?]: Inverse hoisting behaviour
3 participants
@akwodkiewicz @larixer @merceyz