Install transitive dependencies even if listed as devDependencies in the project

[DanReyLop]

Fixes #2819

Go to #2819 for a minimal way to reproduce it. Basically, if you have these kind of dependency tree:
package.json:

{
  dependencies: {
    "a": "1.0.0"
  },
  devDependencies: {
    "b": "1.0.0"
  }
}

a/package.json:

{
  dependencies: {
    "b": "1.0.0"
  }
}

If you install your package using the --production flag, then b won't be installed (and it should, since it's a transitive dependency of a). This PR fixes this.

The issue was that, if the exact same version of a dependency pattern is found multiple times during the resolve phase, only the first time it will actually be resolved. In order to prune devDependencies, all packages that have exactly 1 "resolve request" after the resolve phase will be discarded. I've removed the optimisation that checks if a given pattern has already been resolved, and changed an array to a Set to not impact performance.

This has probably not been a good explanation but I'm not very familiar with this codebase. I've tried to see what was the motivation for that optimisation, but it's there since the creation of this repo.

Test plan

• Create a project like the one described in 'yarn install --production' does not install transitive dependencies if listed as devDependencies in the project #2819, with sequelize-cli as a dependency and gulp as a devDependency.
• Run yarn install --production
• In master, gulp won't be present in node_modules. With this PR, it will be.
• In master, yarn check verify-tree will fail. With this PR it will pass.

[blexrob]

I think this bug originates in the package hoister. In production mode, devDepencies are ignored, but when a subdependency of the normal dependencies is deduped with a root devDependency, that root devdep isn't marked as non-ignored. I'm afraid your solution only works when the two references are exactly the same, but if the versions differ, the requests won't be merged.

[DanReyLop]

@blexrob The hoister bug you are describing seems to have been fixed here: #2537

I've tried installing this package.json using yarn@master:

{
  "name": "test",
  "version": "1.0.0",
  "dependencies": {
    "sequelize-cli": "^2.5.1"
  },
  "devDependencies": {
    "gulp": "^3.9.0"
  }
}

sequelize-cli depends on "gulp": "^3.9.1", so in this case the devDependency and the transitive dependency weren't the exact same version pattern. In this case, it works fine, even without my fix. My fix is just when the devDependency and the transitive dependency is exactly the same.

[blexrob]

@DanReyLop I should have done a test before commenting, sorry for that noise. This PR fixes indeed that when the same version of a (not yet installed) package is referenced multiple times, different references are created for that package, fixing this bug.

However, it does trigger another one with the following package.json:

{
  "name": "test",
  "version": "1.0.0",
  "devDependencies": {
    "gulp": "^3.9.0",
    "citronjs": "0.0.1"
  }
}

(citronjs chosen as the first package in gulp's npm page that has gulp as dependency).

When running yarn install, there are now two references to the newly installed gulp - one from the package.json and one from citronjs. This PR merges those into one request with two references.

The markIgnored function in install.js however, only marks packages with at most one request as ignored - but gulp now has two. And so yarn install --prod will now happily install gulp with this PR applied...

The fix for that would be to let markIgnored just mark all its packages as ignored and let the package hoister deal with the fallout of transitive dependencies. Btw, I wrote #2837 to fix some of the problems in the package hoister with --prod installs, but #2819 was one of the problems I missed there (PR #2921 also fixes #2819 by fixing the package hoister.)

I'll see if I can modify #2921 to include that last fix.

[blexrob]

I have added a commit to #2921 which removes the reference count check in markIgnored. The package hoister automatically includes ignored packages when they are used by another non-ignored package, so it isn't needed anymore.

[DanReyLop]

@blexrob Wow, ok, that went way over my head. The hoister is a beast that I'm not ready to wrestle with (yet).

Great find with that counter-example! Looks like your fix makes this PR obsolete, I'm closing this :)

I have just one small suggestion, please add a test like the one on this PR. Your testing approach with file: dependencies seems way cleaner that mine with the offline-mirror thing, but what I mean is that you should include a test with that dependency tree:

a depends on b
a devDepends on c
b depends on c
--production installs c

And probably should add too the example you just described:

a devDepends on b
a devDepends on c
b depends on c
--production doesn't install c 

There's no such thing as "too many tests" when you're testing the absolute core of the program :)