please move snyk from dependencies to devDependencies

[sod]

Is snyk required to use quagga2? If not, it'll be amazing if it wasn't part of npm install @ericblade/quagga2. I ask because snyk is a pretty large dependency, weighting in at minimum 50 MB, up to 120 MB extra - depending how many of the peers of snyk you already have.

[github-actions[bot]]

Thank you for filing an issue! Please be patient. :-)

[ericblade]

While I don't think it's technically a requirement, it is used as a mechanism to patch some dependencies. I am not positive if it is currently providing any benefit now that most of this project's dependencies have been updated to the latest versions of things.

https://support.snyk.io/hc/en-us/articles/360000911277-Why-has-Snyk-been-added-as-a-dependency-in-my-project-

It's definitely worth a review of a) do we have the latest version of everything we can update, b) are we using any packages that are no longer being updated? are there alternatives? c) does running snyk protect do anything for us presently, and should we move it out to a devdep?

i don't right off hand know the answers to any of these questions, but they'd definitely be worth knowing the answer to.
If snyk is only operating on our devdeps, which most definitely do still have some pretty out of date packages listed, or if snyk isn't doing anything at all for us then we should consider moving it.

[sod]

I didn't indent to ask for removal of snyk from quagga for development or maintenance purpose. Just from the quagga that is published to npm.

If I do:

yarn init -y
yarn set version berry
yarn config set nodeLinker node-modules
yarn add @ericblade/quagga2

then node_modules is 136 MB in size!

When I nullify snyk, as that dependency is just occupied space and doesn't do anything for me, via adding to the package.json:

  "resolutions": {
    "snyk": "https://registry.npmjs.org/@favware/skip-dependency/-/skip-dependency-1.1.1.tgz"
  }

then node_modules is down to 44 MB!

When you look at the package.json of quagga, nearly all of the "dependencies" are actually devDependencies. Most of them are inlined at build time. Only references to get-pixel & ndarray-linear-interpolate remain. So when I force the rest to null:

  "resolutions": {
    "gl-mat2": "https://registry.npmjs.org/@favware/skip-dependency/-/skip-dependency-1.1.1.tgz",
    "gl-vec2": "https://registry.npmjs.org/@favware/skip-dependency/-/skip-dependency-1.1.1.tgz",
    "gl-vec3": "https://registry.npmjs.org/@favware/skip-dependency/-/skip-dependency-1.1.1.tgz",
    "lodash": "https://registry.npmjs.org/@favware/skip-dependency/-/skip-dependency-1.1.1.tgz",
    "ndarray": "https://registry.npmjs.org/@favware/skip-dependency/-/skip-dependency-1.1.1.tgz",
    "snyk": "https://registry.npmjs.org/@favware/skip-dependency/-/skip-dependency-1.1.1.tgz"
  }

my node_modules is down to 18 MB! And it works just like before.

[ericblade]

so, presently after doing npm install on quagga, it should run snyk protect, to patch any dependencies that it has patches for that may not have been updated by their devs, or are updated in incompatible versions. At least, that's what it does if there's any work for it to do, and I don't know if there is or not.

As to the other deps, you do have a few interesting points that i've not really considered in the slightest -- i don't really know 100% what the build step does, so i can't speak to if those modules need to be installed at install time or if they can all be moved to dev deps, due to any inlining that might be happening.

On top of that point, you're bringing me to a realization that some or maybe even all of those modules may only be referenced by the node.js code, which might make them totally irrelevant to browser, and could be a source of additional optimizations. (or there might be no effect at all, if the packaged bundles don't include them because they aren't referenced in the browser code, which hopefully is how it actually works)

So, I think there's a few actions here

1) make sure that all our non dev dependencies are using the latest version, and that those packages are still maintained. If any packages that are regular dependencies are not being maintained anymore, find alternatives that are.

2) move snyk to a devDep since running snyk protect on the user's side shouldn't be desireable if all our regular dependencies are up to date and maintained

3) investigate if the dependencies are actually necessary for an end user to install, or if they are inlined, and move any that might be being inlined into devDep

4) figure out if there's any good way to separate browser deps from node deps (if it is inlining things like gl-vec2, that might be an indication that the original author already thought of this, and set it up to build in the modules that were not needed for both browser and node, rather than actually depending on the user having them...)

and voila, could be a large improvement.

possible way to check this would be to install the source repo, npm install, do a build, remove the step in the tests that builds before running tests, then start removing dependencies and running tests after each one.

i'm concerned, primarily, that not installing the deps you've nulled out could cause breakage in codepaths that you're not going down in your use case.

[ericblade]

are you using node, browser, or both?

[sod]

We only use quagga in the browser.

[icleolion]

2. move snyk to a devDep since running snyk protect on the user's side shouldn't be desireable if all our regular dependencies are up to date and maintained

I use quagga2 in browser also and due to the significant number of dependencies installed I almost decided against using this package. I've never used snyk so apologies if everything from here on in makes no sense, but from what I've read the last couple of hours I don't think snyk is doing what you think it does with how you currently have it hooked up.

You have snyk protect hooked up to the prepublish script. The prepublish script is now deprecated and replaced with prepare (https://docs.npmjs.com/cli/v7/using-npm/scripts#prepare-and-prepublish). Regardless of the deprecation however, snyk is not running on the user's side because prepublish/prepare is only called when you do a local npm install, npm pack or npm publish within this package. Whether prepublish/prepare runs or not is irrelevant however, as all snyk protect will do is install the patches as specified in your .snyk file, which is empty and has been for nearly 2 years, so it will do nothing.

If you want to continue using snyk, then I think you have two options. Either switch snyk to use the postinstall script (which will run when a user installs your package) and keep it as a dependency or switch to using the prepare script and move it to a devDependency.

I personally hope you'll choose the latter. If I want to run snyk, I can install it myself, I'd rather not have a dependency force it on me, especially with the package bloat it brings along with it. Snyk themselves suggest going the prepare route (https://github.com/snyk/snyk/issues/106#issuecomment-333441680). I have my own methods for staying on top of package vulnerabilities.

Seeing as moving snyk to devDependencies is the easiest task with the biggest reward, could that be done first please, and then the other dependencies mentioned above tackled appropriately later on down the line?

[icleolion]

While I don't think it's technically a requirement, it is used as a mechanism to patch some dependencies. I am not positive if it is currently providing any benefit now that most of this project's dependencies have been updated to the latest versions of things.

https://support.snyk.io/hc/en-us/articles/360000911277-Why-has-Snyk-been-added-as-a-dependency-in-my-project-

It's definitely worth a review of a) do we have the latest version of everything we can update, b) are we using any packages that are no longer being updated? are there alternatives? c) does running snyk protect do anything for us presently, and should we move it out to a devdep?

i don't right off hand know the answers to any of these questions, but they'd definitely be worth knowing the answer to. If snyk is only operating on our devdeps, which most definitely do still have some pretty out of date packages listed, or if snyk isn't doing anything at all for us then we should consider moving it.

Figured it might also be useful to answer some of the questions you raised in this post also. With snyk CLI you have test, monitor and protect commands, or you can use wizard which is an interactive version of those three commands. snyk wizard will scan your dependencies and devDependencies and assuming it finds a vulnerability you have four options.

1. Tell snyk to ignore it if you judge it to be a non-issue.
2. Update the package if an updated version with a fix has been released.
3. If it's applicable you can just update the relevant dependency of the package if an updated version with a fix has been released.
4. Install a patch crafted by snyk themselves, which is the route you take if you don't want to update a package due to it maybe bringing along some breaking changes with it, or if an updated version with a fix doesn't exist.

As mentioned above snyk protect run by the prepublish script is not running for users installing your package, and at the moment because your .snyk file is empty it is doing nothing for you either. You'll need to run test/wizard to determine if your current set of dependencies/devDependencies have vulnerabilities. Even if they do, you might be able to just update to newer versions which will mean snyk protect is still redundant.

Funnily enough, I just checked the history of your package.json file and 3 of the last 4 times snyk has raised a PR to fix a vulnerability, it has been to update itself. I assume to update one of it's own vast array of dependencies! Outside of those PRs snyk isn't doing anything that dependabot isn't already doing. And considering all 4 of those vulnerability fix PRs were to update packages, not install snyk patches, staying on top of dependencies with dependabot would have been enough anyway.

[ericblade]

I appreciate the additional analysis. The situation has quite probably changed quite a bit from when I forked initially.

Good news? i've got a long weekend from work right now, perhaps between time spent catching up on house repairs and mandatory computer upgrades, I'll grab some time to take a look at this. It might well be possible that just relying on dependabot would be fine.. the landscape for this sort of stuff has changed drastically in the last couple of years. Maybe i'll get the automated release/patch notes things working correctly too :-)

[icleolion]

I'm glad I could be of some help. By far the best return-on-effort will be moving snyk to devDeps, so if that's all you find time to do I'll still be over the moon and super grateful! I definitely appreciate your time isn't infinite.

[ericblade]

I appreciate the additional help, it definitely helps me to make a movement on something, i made a ton of big changes early on, especially with upgrading deps, when my time was copious, but now with a new job started recently, and a real product to deliver soon, it's become a lot more of a crunch to find time to just validate that this isn't a major change and that my reasoning earlier for keeping this is no longer true.

[ericblade]

i didn't get to it last weekend, but unless work piles me up with stuff that needs to get done this weekend, i've got time actually set aside to handle personal project maintenance this weekend.

[ericblade]

just went ahead and removed it in https://github.com/ericblade/quagga2/commit/c966812ee968ad8a00d85b66d59741149adf0270

also updated all the other safe to update devDeps