plugCubed / plugAPI

A generic API for creating plug.dj bots
MIT License
78 stars 45 forks source link

Reevaluate dependencies #141

Open mortenn opened 7 years ago

mortenn commented 7 years ago

PlugAPI seems to have a few deps that pull in a lot of other deps. The worst ones in this regard are bufferutil and got.

I would recommend looking at their usage and if they can be phased out. I just made the move from request to needle and in our bot, and that was a lot of extra deps removed. Having a lot of dependencies is not an inherently good situation to be in, imo.

thedark1337 commented 7 years ago

For bufferutil , its an optionalDependency, however I could move it to peerDependency if it really is causing that many issues with installation size. got is actually very lightweight compared to request, hence why I moved over to using that for v5.

All told, using cost-of-modules , plugAPI is nowhere near heavy in size.

https://i.tfle.xyz/2017-09-25_22-13-49.png

mortenn commented 7 years ago

if got is an alternative to request, how about needle? I very much liked what I saw when I looked at that. My concern about dependency count has little to do with size, actually.. More to do with potential security flaws and breakability (leftpad, anyone? :p)

samhmills commented 7 years ago

To follow up on this, EventEmitter3 should probably be replaced by the 'events' native module that it originally superseded. With correctly documented events the necessity for listening to all events in one listener and the usage of .once isn't substantial enough in my opinion.

This would be a breaking change however, forcing an update to v6, not that that should stop it.

anjanms commented 7 years ago

+1 for using native events. As it stands now, there is no all event in EE3 anyway. And the native module does support once.

thedark1337 commented 7 years ago

The reason why Eventemitter3 is used is because of the performance improvements that it has over native events. The methods are the same as native events with major differences. The reason why Got is used is because it works perfectly fine and it hasn't shown to have any issues at all as of yet. The main changes I could do is move bufferutil and utf-8-validate to peerDependency. As those are not actually 100% necessary, only to improve performance of ws.

All in all though, I don't see any of the dependencies we use ever being unpublished. That leftpad incident will not ever happen again as NPM has changed their policies to disallow that from happening. More info can be found here: http://blog.npmjs.org/post/141905368000/changes-to-npms-unpublish-policy

FranciscoG commented 4 years ago

I like got, I'm using it as well

But, speaking of bufferutil, one of it's sub deps has a high security vulnerability

# Run  npm update bl --depth 6  to resolve 2 vulnerabilities
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Remote Memory Exposure                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ bl                                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ plugapi                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ plugapi > bufferutil > prebuild-install > tar-fs >           │
│               │ tar-stream > bl                                              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/1555                            │
└───────────────┴──────────────────────────────────────────────────────────────┘

Current version of bufferutil is 4.0.1 PlugAPI is still has an older version listed in its package.json "bufferutil": "2.x || 3.x",

bufferutil has switched from prebuild-instal to node-gyp-prebuild so it should solve this security problem.

FranciscoG commented 4 years ago

and also utf-8-validate

current version: 5.0.2 PlugAPI optional dep version: "utf-8-validate": "2.x || 4.x"

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Remote Memory Exposure                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ bl                                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ plugapi                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ plugapi > utf-8-validate > prebuild-install > tar-fs >       │
│               │ tar-stream > bl                                              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/1555                            │
└───────────────┴──────────────────────────────────────────────────────────────┘

it has also switched from prebuild-instal to node-gyp-prebuild so it should solve this security problem.