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

fix(resolution): use registry latest if it satisfies requested semver range #4804

Merged
merged 2 commits into from Oct 31, 2017

Conversation

rally25rs
Copy link
Contributor

@rally25rs rally25rs commented Oct 30, 2017

Summary

Fixes #3560.
Related PR: #4797.

Mimic behavior in NPM; use the latest version in registry if it
satisfies the semver range requests.

Otherwise fallback to semver.maxSatisfying()

NPM source that specifies this behavior is: https://github.com/npm/npm/blob/d46015256941ddfff1463338e3e2f8f77624a1ff/lib/utils/pick-manifest-from-registry-metadata.js#L10-L14

I believe this is OK directly in the NPM resolver, as other resolvers may not have a "latest" tag or dist-tags in general (for example, I don't believe git-resolver has this metadata to check).

Test Plan

Added unit test to verify behavior.

… range

Fixes yarnpkg#3560

**Summary**

Mimic behavior in NPM; use the `latest` version in registry if it
satisfies the semver range requests.

Otherwise fallback to `semver.maxSatisfying()`

**Test Plan**

Added unit test to verify behavior.
@buildsize
Copy link

buildsize bot commented Oct 30, 2017

This change will increase the build size from 9.94 MB to 9.94 MB, an increase of 554 bytes (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 859.9 KB 859.72 KB -184 bytes (0%)
yarn-[version].js 3.78 MB 3.78 MB 438 bytes (0%)
yarn-legacy-[version].js 3.83 MB 3.83 MB 438 bytes (0%)
yarn-v[version].tar.gz 865.48 KB 865.1 KB -386 bytes (0%)
yarn_[version]all.deb 654.43 KB 654.67 KB 248 bytes (0%)

@arcanis arcanis self-assigned this Oct 30, 2017
neonowy added a commit to neonowy/yarn that referenced this pull request Oct 30, 2017
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -986,3 +986,42 @@ test.concurrent('preserves unaffected bin links after adding to workspace packag
expect(await fs.exists(`${config.cwd}/packages/workspace-2/node_modules/.bin/workspace-1`)).toEqual(true);
});
});

test.concurrent('installs "latest" instead of maxSatisfying if it satisfies requested pattern', (): Promise<void> => {
/* eslint-disable */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just disable the relevant warning with /* eslint-disable rule-name */ instead of a blanket disable statement in case the enable part goes away?

Or ideally, we wouldn't need this at all since you can use https://git.io/ to get short GitHub URLs ;)

@rally25rs rally25rs merged commit 1c845bd into yarnpkg:master Oct 31, 2017
@rally25rs rally25rs deleted the check-latest-before-max-3560 branch October 31, 2017 15:49
arcanis pushed a commit that referenced this pull request Oct 31, 2017
neonowy added a commit to neonowy/yarn that referenced this pull request Oct 31, 2017
arcanis pushed a commit that referenced this pull request Oct 31, 2017
…one (#4797)

* fix(info): Use version from `latest` dist-tag instead of the highest one

Fixes #3947. By default, package `version` was set by sorting all the versions and getting the highest
one. Now it's provided via package `latest` dist-tag.

* Fix linter issues by shortening the test description

* Manually mock request

* Add scenario comment from #4804
calvinhuang pushed a commit to calvinhuang/yarn that referenced this pull request Nov 9, 2017
… range (yarnpkg#4804)

* fix(resolution): use registry latest if it satisfies requested semver range

Fixes yarnpkg#3560

**Summary**

Mimic behavior in NPM; use the `latest` version in registry if it
satisfies the semver range requests.

Otherwise fallback to `semver.maxSatisfying()`

**Test Plan**

Added unit test to verify behavior.

* fix eslint line len, additional field check
calvinhuang pushed a commit to calvinhuang/yarn that referenced this pull request Nov 9, 2017
…one (yarnpkg#4797)

* fix(info): Use version from `latest` dist-tag instead of the highest one

Fixes yarnpkg#3947. By default, package `version` was set by sorting all the versions and getting the highest
one. Now it's provided via package `latest` dist-tag.

* Fix linter issues by shortening the test description

* Manually mock request

* Add scenario comment from yarnpkg#4804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yarn installs wrong version
3 participants