Snyk brings in a boat-load of dependencies

[JaredReisinger]

A bit puzzled here. Why would this package need so many dependencies?

[4/4] Building fresh packages...
success Saved lockfile.
success Saved 187 new dependencies.
info Direct dependencies
└─ @jaredreisinger/react-crossword@2.2.3
info All dependencies
├─ @arcanis/slice-ansi@1.0.2
├─ @emotion/is-prop-valid@0.8.8
├─ @jaredreisinger/react-crossword@2.2.3
├─ @nodelib/fs.scandir@2.1.3
├─ @nodelib/fs.stat@2.0.3
├─ @nodelib/fs.walk@1.2.4
├─ @octetstream/promisify@2.0.2
├─ @sindresorhus/is@3.1.0
├─ @snyk/cli-interface@2.8.1
├─ @snyk/cocoapods-lockfile-parser@3.4.0
├─ @snyk/composer-lockfile-parser@1.4.0
├─ @snyk/dep-graph@1.19.3
├─ @snyk/docker-registry-v2-client@1.13.5
├─ @snyk/gemfile@1.2.0
├─ @snyk/inquirer@6.2.2-patch
├─ @snyk/java-call-graph-builder@1.13.1
├─ @snyk/rpm-parser@2.0.0
├─ @snyk/ruby-semver@2.2.0
├─ @snyk/snyk-cocoapods-plugin@2.3.0
├─ @snyk/snyk-docker-pull@3.2.0
├─ @szmarczak/http-timer@4.0.5
├─ @types/cacheable-request@6.0.1
├─ @types/debug@4.1.5
├─ @types/emscripten@1.39.4
├─ @types/glob@7.1.3
├─ @types/hosted-git-info@2.7.0
├─ @types/http-cache-semantics@4.0.0
├─ @types/js-yaml@3.12.5
├─ @types/keyv@3.1.1
├─ @types/minimatch@3.0.3
├─ @types/responselike@1.0.0
├─ @types/semver@5.5.0
├─ @types/xml2js@0.4.5
├─ @yarnpkg/core@2.1.1
├─ @yarnpkg/json-proxy@2.1.0
├─ @yarnpkg/lockfile@1.1.0
├─ @yarnpkg/pnp@2.1.0
├─ @yarnpkg/shell@2.1.0
├─ ansi-align@3.0.0
├─ ansicolors@0.3.2
├─ any-promise@1.3.0
├─ archy@1.0.0
├─ ast-types@0.13.3
├─ async@1.5.2
├─ babel-plugin-styled-components@1.11.1
├─ bl@4.0.2
├─ boxen@4.2.0
├─ cacheable-lookup@5.0.3
├─ cacheable-request@7.0.1
├─ camelize@1.0.0
├─ child-process@1.0.2
├─ cli-boxes@2.2.0
├─ cli-spinner@0.2.10
├─ crypto-random-string@2.0.0
├─ css-color-keywords@1.0.0
├─ css-to-react-native@3.0.0
├─ data-uri-to-buffer@1.2.0
├─ decompress-response@6.0.0
├─ defer-to-connect@2.0.0
├─ degenerator@1.0.4
├─ docker-modem@2.1.3
├─ dockerfile-ast@0.0.19
├─ dot-prop@5.2.0
├─ dotnet-deps-parser@4.10.0
├─ duplexer3@0.1.4
├─ email-validator@2.0.4
├─ es6-promise@4.2.8
├─ escape-goat@2.1.1
├─ fast-glob@3.2.4
├─ fastq@1.8.0
├─ file-uri-to-path@1.0.0
├─ fs-constants@1.0.0
├─ ftp@0.3.10
├─ get-uri@2.0.4
├─ global-dirs@2.0.1
├─ got@11.5.2
├─ grapheme-splitter@1.0.4
├─ gunzip-maybe@1.4.2
├─ has-yarn@2.1.0
├─ http2-wrapper@1.0.0-beta.5.2
├─ immediate@3.0.6
├─ immer@7.0.7
├─ import-lazy@2.1.0
├─ is-deflate@1.0.0
├─ is-gzip@1.0.0
├─ is-installed-globally@0.3.2
├─ is-npm@4.0.0
├─ is-yarn-global@0.3.0
├─ json-buffer@3.0.1
├─ json-file-plus@3.3.1
├─ jszip@3.3.0
├─ keyv@4.0.1
├─ latest-version@5.1.0
├─ lodash.assignin@4.2.0
├─ lodash.constant@3.0.0
├─ lodash.escaperegexp@4.1.2
├─ lodash.filter@4.6.0
├─ lodash.flatmap@4.5.0
├─ lodash.foreach@4.5.0
├─ lodash.get@4.4.2
├─ lodash.has@4.5.2
├─ lodash.isarray@4.0.0
├─ lodash.isfunction@3.0.9
├─ lodash.isundefined@3.0.1
├─ lodash.keys@4.2.0
├─ lodash.map@4.6.0
├─ lodash.merge@4.6.2
├─ lodash.reduce@4.6.0
├─ lodash.size@4.2.0
├─ lodash.topairs@4.3.0
├─ lodash.transform@4.6.0
├─ lodash.union@4.6.0
├─ lodash.values@4.3.0
├─ logic-solver@2.0.1
├─ macos-release@2.4.1
├─ netmask@1.0.6
├─ node.extend@2.0.2
├─ os-name@3.1.0
├─ p-cancelable@2.0.0
├─ pac-proxy-agent@3.0.1
├─ pac-resolver@3.0.0
├─ package-json@6.5.0
├─ parse-link-header@1.0.1
├─ peek-stream@1.1.3
├─ picomatch@2.2.2
├─ pluralize@7.0.0
├─ promise-deferred@2.0.3
├─ promise-fs@2.1.1
├─ promiseback@2.0.3
├─ proxy-agent@3.1.1
├─ pupa@2.0.1
├─ quick-lru@5.1.1
├─ registry-auth-token@4.2.0
├─ registry-url@5.1.0
├─ resolve-alpn@1.0.0
├─ reusify@1.0.4
├─ run-parallel@1.1.9
├─ secure-keys@1.0.0
├─ semver-diff@3.1.1
├─ smart-buffer@4.1.0
├─ snyk-docker-plugin@3.16.0
├─ snyk-go-parser@1.4.1
├─ snyk-go-plugin@1.16.0
├─ snyk-gradle-plugin@3.5.1
├─ snyk-module@3.1.0
├─ snyk-mvn-plugin@2.19.1
├─ snyk-nodejs-lockfile-parser@1.26.3
├─ snyk-nuget-plugin@1.18.1
├─ snyk-paket-parser@1.6.0
├─ snyk-php-plugin@1.9.0
├─ snyk-policy@1.14.1
├─ snyk-python-plugin@1.17.1
├─ snyk-resolve-deps@4.4.0
├─ snyk-resolve@1.1.0
├─ snyk-sbt-plugin@2.11.0
├─ snyk-try-require@1.3.1
├─ snyk@1.373.0
├─ socks@2.3.3
├─ split-ca@1.0.1
├─ ssh2-streams@0.4.10
├─ ssh2@0.8.9
├─ stream-buffers@3.0.2
├─ stream-to-array@2.3.0
├─ stream-to-promise@2.2.0
├─ streamsearch@0.1.2
├─ styled-components@5.1.1
├─ tar-stream@2.1.3
├─ temp-dir@1.0.0
├─ tempfile@2.0.0
├─ term-size@2.2.0
├─ thunkify@2.1.2
├─ to-readable-stream@1.0.0
├─ toml@3.0.0
├─ tree-kill@1.2.2
├─ tunnel@0.0.6
├─ typedarray-to-buffer@3.1.5
├─ underscore@1.10.2
├─ unique-string@2.0.0
├─ update-notifier@4.1.0
├─ url-parse-lax@3.0.0
├─ vscode-languageserver-types@3.15.1
├─ widest-line@3.1.0
├─ window-size@0.1.4
├─ windows-release@3.3.3
├─ xml2js@0.4.23
├─ xmlbuilder@11.0.1
└─ yaml@1.10.0

Originally posted by @zehawki in #31 (comment)

[JaredReisinger]

Looking into Snyk a bit, it appears that one of the things they try to do is perform post-install patching of dependencies to fix known vulnerability issues. To that end, they added a prepare npm script (which runs after install, even when @jaredreisinger/react-crossword is just a dependency) that runs the snyk protect command. For that to work, snyk has to be installed.

I have mixed feelings about this. On one hand, 187 new dependencies is an absolutely absurd number of dependencies to add. (But note that npm dependencies tend to explode all over the place anyway; it's a difficult challenge to overcome, especially when it comes to React/babel/webpack projects.) On the other hand, these dependencies should not impact a built production React app, since only the JS actually used in the app gets bundled into the final static JS file.

As I see it, there are two options:

1. Let Snyk continue to provide "just-in-time" vulnerability patching, at the cost of a larger npm install cost (but one that should not adversely impact final app size)
2. Turn off "just-in-time" Snyk vulnerability patching, keeping the npm install cost low. Snyk will continue to monitor the repo out-of-band, and provide PRs to update packages in react-crossword, but will be limited to only using published dependency packages.

As this is a decision that impacts react-crossword's consumers more than myself, I'd love to hear your opinions. @zehawki, what's your preference?

[JaredReisinger]

Found a description of how/why this works the way it does: https://support.snyk.io/hc/en-us/articles/360003891078-Snyk-patches-to-fix-vulnerabilities

[JaredReisinger]

So.... as you can tell by the pending PR, I'm leaning pretty heavily towards removing snyk as a full dependency... it just feels too heavy and opinionated for a single component to bring in. As the PR says, there's nothing stopping a consumer from choosing to use Snyk to patch vulnerabilities... and that's the "correct" level at which to make that decision.

I'm still going to sit on the PR for a day or so, in case anyone has an incredibly strong opinion to keep snyk in as a regular dependency. (Even after that day, I can always change it back... yay for source control!)

[zehawki]

I'm just trying to understand whats going on with the dependencies and there's probably lots I dont know, so feel free to correct me.

If Synk is patching vulnerabilities, I would assume that its patching them in packages that are already installed, right? Not adding new ones. To discover that, I'm running yarn list --pattern <xyz> to see whether a package is installed.

I have multiple build environments, so I checked the one in which I yarn added this package (B) vs an earlier one (A). The only thing different between the 2 environments is the crossword package.

A: yarn list --pattern pac-resolver found nothing
B: yarn list --pattern pac-resolver found
yarn list v1.7.0
└─ pac-resolver@3.0.0

A: yarn list --pattern decompress-response found nothing
B: yarn list --pattern decompress-response found
yarn list v1.7.0
├─ decompress-response@6.0.0
└─ package-json@6.5.0
└─ decompress-response@3.3.0

A. yarn list --pattern lodash.reduce found nothing
B. yarn list --pattern lodash.reduce found
yarn list v1.7.0
└─ lodash.reduce@4.6.0

And so on. These 3 were selected at random from the 187 dep list. So it looks like new packages were installed.

Then I went to searched in https://snyk.io/vuln/ with many of the names from above and didnt get any vulnerabilities reported... so not sure whats going on.

[JaredReisinger]

Yes and no... 😄 You've just discovered the issue of transitive dependencies.

When it patches vulnerabilities, yes, Snyk actually modifies the existing dependencies in-place in your node_modules directory. The patching itself does not introduce any new dependencies.

However, snyk itself brings in an additional 184 dependencies in order to:

• provide its own CLI tool
• iterate your node_modules directory
• read its .snyk config file
• perform the patching

Another way to verify this is by looking at the direct dependencies I added: immer, styled-components, and snyk. If you look at the package.json files for the first two, you'll see that they themselves have devDependencies, but no regular dependencies at all! This means that when you install react-crossword, you'll get immer and styled-components, and nothing else additional. Compare this with snyk's package.json, which lists 44 direct dependencies. Those are all new transitive dependencies for react-crossword, and may (and do!) include their own additional dependencies, eventually adding a total of 185 new dependencies (synk itself plus 184 additional transitive dependencies).

So anyway, it's not the behavior of snyk that's ballooning the dependencies, it's the mere existence of snyk as a non-dev dependency of react-crossword. (What’s really wrong with node_modules and why this is your fault is a good read about this, keeping in mind that the "you" in the title isn't you, it's the npm developer ecosystem at large. This is my motivation to move snyk back to a devDependency... at least I won't be contributing to the explosion of others' node_modules directories!)

[JaredReisinger]

Unmentioned in my previous comment is a behavior of npm and yarn you might not expect. The devDependencies (and any of their transitive dependencies) are only installed for the "main" package.json (when you npm install). When you install a dependent package (npm install @jaredreisinger/react-crossword, or anything in the top-level dependencies or devDependencies lists) you get their dependencies, but not the devDependencies. The rationale here is that you should only get the devDependencies when you're "in" that package itself, doing development work. Otherwise, they are completely unneeded. By moving snyk into devDependencies, I will pay the cost of downloading it so I can use its CLI to analyze for vulnerabilities. But consumers of react-crossword will not have to download it (and it's gigantic pile of transitive dependencies).

[zehawki]

Cant imagine why synk needs a million packages to do something like just patching some files, but OK, the bloat of JS & node_modules is real. Thank goodness for production build and tree shaking etc.

I keep tight control over our production build to ensure that nothing adds bytes to the critical path chunks. So I looked though various chunks generated before and after this package, and I see no adverse effect. The chunk containing crossword has styled-components (20KB), immer (15.2), crossword (13.4), emotion (12.8). Since thats on its own route, its no real issue, but if there's a way to remove styled-components, pls let me know since I don't intend to use it.

As a side note bundlephobia was spot on. I always check there before adding a new package.

TL;DR: I guess there's no real problem, but maybe adding synk as a dev dependency is the way to do.

Thanks for the detailed notes above and the hackernoon read is excellent.

[JaredReisinger]

Both immer and styled-components are necessary runtime dependencies, as react-crossword depends on them. I could try bundling them into the react-crossword distribution, but that wouldn't impact the actual size of react-crossword.

Since I'm not hugely concerned with vulnerabilities in react-crossword (there's a very limited scope for which it takes user input, and it handles it in a very specific way), I'm going to go ahead and merge PR #34. If somehow this package starts getting used in all React apps everywhere (crosswords on all websites!), and there's evidence that it's being used a vulnerability vector... well then, at that point the issue can be revisited. 😁

Thanks again for catching the dependency bloat!