Adding new dependency does not honor existing version constraint in resolution

[hniirane]

Do you want to request a feature or report a bug?
Bug.

What is the current behavior?
Yarn does not honor already added version locks when resolving the dependencies of newly added dependencies.

If the current behavior is a bug, please provide the steps to reproduce.

1. Init an empty module
2. Add react 15.6.1 yarn add react@15.6.1
3. Add storybook-router@0.2.9 yarn add storybook-router@0.2.9
4. The bug reproduces and two react different versions are installed:

$ yarn list react
yarn list v1.1.0
├─ react@15.6.1
└─ storybook-router@0.2.9
   └─ react@15.6.2
✨  Done in 0.24s.

What is the expected behavior?
Expected only react@15.6.1 to be installed as that can fulfill all version constraints. This is what happens if I delete the yarn.lock after both dependencies are in the package.json and yarn install i.e.

$ rm -rf node_modules/ yarn.lock && yarn install && yarn list react
yarn list v1.1.0
└─ react@15.6.1
✨  Done in 0.17s.

Please mention your node.js, yarn and operating system version.

$ node --version
v6.9.4
$ yarn --version
1.2.0
$ system_profiler SPSoftwareDataType|grep "System Version"
      System Version: macOS 10.12.6 (16G29)

EDIT: The original repro was done with yarn v.1.1.0, but I upgraded to 1.2.0 and this reproduces still.

[rally25rs]

I'm digging into this...

Noticing that the parameters passed to PackageResolver.getHighestRangeVersionMatch(name, range, manifest) might be incorrect. The passed range seems to be the actual resolved single version (15.6.2) instead of the requested range (^15.5.4).

That in turn builds up to the line where it checks to see if any current installed versions match the semver range: semver.maxSatisfying(["15.6.1"], '15.6.2'); // null If the correct range had been passed, then it would have executed semver.maxSatisfying(["15.6.1"], '^15.5.4'); // 15.6.1 which should solve this issue.

I'll continue backtracking through the code...

[rally25rs]

OK it looks like this was introduced by #3729 specifically the line in package-request.js:

const solvedRange = semver.validRange(range) ? info.version : range;

So if the range (^15.5.4) is valid, then we use info.version instead (15.6.2).
That gets passed down to see if any installed versions match 15.6.2:

const maxValidRange = semver.maxSatisfying(['15.6.1'], '15.6.2'); // null

@arcanis it looks like that linked PR was yours. I'm hesitant to change anything and risk re-opening whatever that PR fixed. Could you lend a hand?

[arcanis]

Will look into this today 👍

[arcanis]

Sooo, let's try to fix that! I don't like that packages are relying on hoisting (mark my words - even if we fix this, they will break again in the future), but us making it less efficient than it could be isn't great either. Reposting what I put on #5561:

I don't really understand the above logic. if range is a valid semver then don't use it?

You're right, that doesn't make a lot of sense :/ I think what I wanted to do was "if this is not a version but is a range, then use the one we already have if possible" (since the goal of the PR was to not do optimization when reading from the lockfile). As such the condition should probably be:

const solvedRange = semver.valid(range) && !semver.validRange(range) ? range : info.version;