Skip to content
This repository has been archived by the owner on Jul 3, 2021. It is now read-only.

Reevaluate dependencies #141

Open
mortenn opened this issue Sep 26, 2017 · 7 comments
Open

Reevaluate dependencies #141

mortenn opened this issue Sep 26, 2017 · 7 comments
Labels

Comments

@mortenn
Copy link
Contributor

mortenn commented Sep 26, 2017

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
Copy link
Member

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
Copy link
Contributor Author

mortenn commented Sep 26, 2017

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
Copy link
Member

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
Copy link
Contributor

anjanms commented Sep 26, 2017

+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
Copy link
Member

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
Copy link

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
Copy link

FranciscoG commented Sep 27, 2020

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants