fix: support optional peerDependencies with npm7

[louis-bompart]

• [x] Tests written and linted
• [x] Documentation written / README.md updated https://snyk.io/docs/snyk-for-node/; Not Applicable
• [x] Follows CONTRIBUTING agreement;
• [x] Commit history is tidy https://git-scm.com/book/en/v2/Git-Branching-Rebasing; test are split on purpose to showcase a fail build.
• [x] Reviewed by Snyk team

What this does

This PR aims to correct that by excluding the optional peer dependencies from the generated top-level dependency tree.

Notes for the reviewer

• Currently, Snyk considers that a peer dependency should be part of the lockfile if there is such dependency and the lockfileVersion !==2. This does not take into account the peerDependencyMeta and the possibility that a peer dependency can be optional (see npm doc).
• The proposed behaviour assumes that Snyk should 'dismiss' optional peer dependency.
• Should fix #121

More information

• npm, peerDependenciesMeta.

Screenshots

Visuals that may help the reviewer

[CLAassistant]

All committers have signed the CLA.

[louis-bompart]

Hey @JamesPatrickGill, thanks for your fast response!

I modified the target of this PR to the branch you mentioned and squash the commits ;) If you need anything else about this PR, feel free to reach out 😄

[louis-bompart]

Hi @JamesPatrickGill, is there anything holding this PR up?

[JamesPatrickGill]

Hi again, sorry for the delay on this, this is definitely still code we want to see merged. There is a couple of additions we would like to add to this ticket though.

1. Upon testing I can see this does indeed stop the problem with optional peerDeps that are uninstalled 😃 - though it leads to the behaviour where if the optional dependency is installed, it is ignored anyway.

In my opinion we could fix this quickly by passing the deps found via parsing the lockfile to getTopLevelDeps and add an additional check to the if check you added. It could look like this:

lock-parser-base.ts

...

const topLevelDeps: Dep[] = getTopLevelDeps({
    targetFile: manifestFile,
    installedDeps: Object.keys(depMap),
    includeDev,
    includePeerDeps: lockfile.type === LockfileType.npm7,
    applyYarn2Resolutions: lockfile.type === LockfileType.yarn2,
});

...

lib/parsers/index.ts

...

export function getTopLevelDeps({
  targetFile,
  installedDeps,
  includeDev,
  includePeerDeps = false,
  applyYarn2Resolutions = false,
}: {
  targetFile: ManifestFile;
  installedDeps: string[];
  includeDev: boolean;
  includePeerDeps?: boolean;
  applyYarn2Resolutions?: boolean;
}): Dep[] {

...

if (
  targetFile?.peerDependenciesMeta?.[name]?.optional &&
  !installedDeps.find((depName) => depName === name)
) {
  continue;
}

...

This is the quickest way I can see of implementing this anyhow. However if I have misunderstood something about the behaviour please ask any questions.

2. In addition to the above, a test fixture to cover both cases would be ideal:
   • Optional peer dep not installed therefore not scanned by snyk
   • Optional peer dep is installed and hence is scanned by snyk

Thank you for your contribution again 😄 , I hope this provides you enough information to understand and make the proposed changes.

[louis-bompart]

Hey @JamesPatrickGill , thanks for your review, that's really appreciated and sorry for this tardive follow-up.

I did not manage to reproduce the issue you described and updated the tests included in this PR to reflect that and be more exhaustive overall.

If an optional peer dependency is actually installed, then it must be also set either in the dev dependencies or the 'prod' dependencies. This is then handled before my changes by this bit of code

  for (const [name, version] of dependenciesIterator) {
    dependencies.push({
      dev:
        includeDev && targetFile.devDependencies
          ? !!targetFile.devDependencies[name]
          : false,
      name,
      version,
    });
  }

If anything, the proposed changeset avoid duplicates in the dependencies array.

I might be missing another case that you had in mind, if so, would you mind detailing it please, so I can try to reproduce it in my tests and fix it?

Thanks again for your time 🙌

[louis-bompart]

Hi @JamesPatrickGill, sorry to @ you again. Did you have the chance to review my last message & the new tests?

[JamesPatrickGill]

Hi again @louis-bompart, thanks for the added test coverage and explanation, this is great!

I am happy to get this merged 🎉

Just one final thing, could you please squash your commits again into your first. Once thats done I will merge into the branch I created and then let CI run before I merge into main.

Thanks again for helping us out 😄

[snyksec]

:tada: This PR is included in version 1.37.2 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: