Correctly label packages as dev-only in yarn audit

[as3richa]

Summary

yarn audit currently does not distinguish packages that are depended on only in dev versus packages that are depended on in prod (in particular it doesn't send a dev key in the payload for the registry audit API endpoint).

Because of this, the registry endpoint implicitly assumes that every package is depended on in prod, and yields dev: false for every finding of every advisory it uncovers, even if that instance of the package is only a (transitive) dev dependency. yarn audit --json basically echos the response verbatim, hence those findings are incorrectly labelled as affecting production in the output.

In order to fix this, I introduced some plumbing to PackageHoister, PackageLinker, and buildTree from hoisted-tree-builder.js to allow hoisted dependencies to be annotated as being depended upon only by dev packages. By sending this information as part of the audit payload, we get the correct result from the registry API for free.

The plumbing I introduced would probably be useful in other commands, e.g. yarn find doesn't seem to distinguish dev and prod dependencies (and I would argue that it would be useful if it did). Moreover, it might also be useful to include the dev-vs.-prod status of an advisory in the human-readable output of yarn audit (whereas my PR adds it only to --json). I didn't go down those rabbit holes in the interest of keeping this PR focused.

Test plan

Running yarn audit --json and trimming out the noise:

1.13.0

{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 535,
      "path": "mime",
      "dev": false,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "1.4.0",
          "paths": [
            "mime"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 535,
      "created": "2017-09-25T19:02:28.152Z",
      "updated": "2018-04-09T00:38:22.785Z",
      "deleted": null,
      "title": "Regular Expression Denial of Service",
      "module_name": "mime"
    }
  }
}
{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 566,
      "path": "hoek",
      "dev": false,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "4.2.0",
          "paths": [
            "hoek"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 566,
      "created": "2018-04-20T21:25:58.421Z",
      "updated": "2018-04-20T21:25:58.421Z",
      "deleted": null,
      "title": "Prototype pollution",
      "module_name": "hoek"
    }
  }
}

My PR

{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 535,
      "path": "mime",
      "dev": false,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "1.4.0",
          "paths": [
            "mime"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 535,
      "created": "2017-09-25T19:02:28.152Z",
      "updated": "2018-04-09T00:38:22.785Z",
      "deleted": null,
      "title": "Regular Expression Denial of Service",
      "module_name": "mime"
    }
  }
}
{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 566,
      "path": "hoek",
      "dev": true,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "4.2.0",
          "paths": [
            "hoek"
          ],
          "dev": true,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 566,
      "created": "2018-04-20T21:25:58.421Z",
      "updated": "2018-04-20T21:25:58.421Z",
      "deleted": null,
      "title": "Prototype pollution",
      "module_name": "hoek"
    }
  }
}

In the first case, the second advisory's findings are incorrectly labelled as affecting production; my PR demonstrates the correct behavior.

[buildsize]

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	1.11 MB	1.11 MB	1.53 KB (0%)
yarn-[version].js	4.47 MB	4.47 MB	4.06 KB (0%)
yarn-legacy-[version].js	4.66 MB	4.67 MB	4.32 KB (0%)
yarn-v[version].tar.gz	1.12 MB	1.12 MB	834 bytes (0%)
yarn_[version]all.deb	815.6 KB	816.04 KB	448 bytes (0%)

[as3richa]

@arcanis Was it a deliberate design decision that yarn.lock doesn't indicate which packages are dev-only? npm's package-lock.json does contain that information.

If it was deliberate decision, is there any context that would help me understand the reasoning behind it? If not, would you accept a PR that computed the dev-onlyness of the tree at install time and stored that information in the lockfile?

The approach I took in this PR works okay, but it feels a little intrusive. I get the impression that yarn wants to treat packages symmetrically irrespective of whether they're dev-only or prod, but sometimes that information ends up being relevant to the human operator, I think this feature (and others) might be implemented more cleanly if we just stored that info in the lockfile.

[arcanis]

Packages aren't marked as dev or not dev because the lockfile only contains the information needed for the tree to be resolved, regardless of the environment (which also is the reason why, when the lockfile isn't present, dev dependencies still have to be resolved even when using --dev). I don't think we should change the way it works.

[as3richa]

Okay, that's fair. Any thoughts on this patch?

[as3richa]

@arcanis Bump

[arcanis]

I would refrain from merging this PR. While I understand the user story, the current implementation complexifies package-hoister which already is too complex for my taste. The maintenance cost is simply too high for the value provided.

[as3richa]

@arcanis I agree that the implementation is pretty hairy. I built it into the hoister because I felt that that was the best place for it, semantically speaking, but it was hard to shoehorn the relevant functionality into the existing interfaces.

I can see an alternative implementation that doesn't implicate the hoister. It's possible to determine which packages are strictly dev-only given just the lockfile and a list of top-level prod dependencies (with a fairly simple graph algorithm). Would you consider a patch in which I implemented the relevant algorithm in a separate file?

[as3richa]

@arcanis I opened #6970 which implements the same feature a bit more cleanly. I would appreciate a review 😁