fix(npm): optional dependencies fix

[0-vortex]

Peer dependencies disable npm shrinkwrap unless they are installed - rendering the dependencies as effectively NOT optional.

[devsnek]

we use peer deps because optional deps will be installed unless you use --no-optional, and in effect, they better fill the definition of peer deps

[0-vortex]

With optionalDependencies and without the --no-optional flag they issue a warning, you are still able to deploy a production configuration using npm shrinkwrap.
With peerDependencies a user needs to manually install them and they will get locked to the shrinkwrap file - this is not something you can call optional. If that is the intended behaviour then it should be reflected in the readme with steps to workaround this issue.

In contrast, the solution I am proposing is zero configuration, it doesn't break anything in NPM 4 and aligns the package to the default NPM usage as documented in the command line and https://www.npmjs.com/

[devsnek]

we have already discussed this throughly

[0-vortex]

Just adding a test case here:

npm i discord.js -S
npm shrinkwrap

There is no way to ignore peerDependencies unless you resolve them.
The above scenario works with the proposed changes.

Tested with npm@4.1.2 and node@7.5.0

[devsnek]

do you need to shrinkwrap that badly?

[0-vortex]

Unfortunately there really aren't any sane alternatives to npm shrinkwrap. I can document the process in the readme if you want to

[amishshah]

It does make sense that they should become truly "optional" dependencies, but yes it does mean they won't be installed. I'm personally ok with the change (it will stop many questions about the installation errors as well). Can anyone else weigh in?

[aemino]

I rather dislike all of the warn errors every time I update a package. The real issue, in my opinion, is that NPM needs better support for peerDependencies. For the time being, however, I think it appropriate that we switch to optionalDependencies. The most common side effect of this change will likely be people complaining about how installing didn't work, to which we will just have to point out that --no-optional should be used. Perhaps we can do some work to make this amply clear on the README.

[Gawdl3y]

Using optionalDependencies isn't really an option, since NPM attempts to install all of them by default.

• We definitely don't want uws to be installed by default
• uws and bufferutil are mutually exclusive
• opusscript and node-opus are mutually exclusive
• Optional dependencies are all or nothing
• Every single one of these (except opusscript) is a native module, taking non-trivial amounts of time to build, and failing on systems that can't build them properly (a significant chunk of our users)
• Just about everybody forgets --no-optional at least 50% of the time, including myself. We have a huge chunk of users (probably the vast majority) that aren't well-versed in this subject matter, and are guaranteed to come to us when they inevitably forget it and see a huge wall of build errors.

Use yarn if you want to lock your dependencies to specific versions. NPM's optional dependency support is hot garbage.

[0-vortex]

I don't mind this turning into a discussion as long as the discussion is reasonable in provided solution.

Right now, your biggest nightmares of having all those modules installed in the dependency tree is a requirement for any bot to be deployed in a stable, production environment. I understand the need for non-breaking changes but right now this package is incomplete - not deployable without everything in peerDependencies installed.

Do you just don't want people to build bots using this framework ?

[iCrawl]

There are a lot of people who built production ready bots based on this framework/library already.

But you might have to enlighten me how peerDependencies make this package incomplete and not deployable without everything in the peerDependencies installed?

The issue with putting everything into optionalDependencies is that, as explained above your comment, some things simply cannot coexist with others.

[0-vortex]

@iCrawl https://docs.npmjs.com/

I have made a simple fix. For a simple problem. It is true peerDependencies and optionalDependencies are incomplete in terms of user experience. It is also true they changed a bit across versions.

Facts:

• NPM@4 can't shrinkwrap this package
• NPM is the de-facto package manager for NODE
• I am only pointing out issues related to the NODE and NPM documentation that do not WORK right now.

I understand some of your users have difficulty using NPM as it was intended and they need to receive this information in a manner that will make them pay attention to the defaults.
Now, I have spent a lot of time trying to figure this out and THIS solution was the only one that I could come up with that still keeps those 6 packages (uws, bufferutil, etc) in the same repository. It seemed reasonable to me to make the required changes and ask how to amend the documentation.

In light of all the excess discussion though I am wondering if those peerDependencies shouldn't actually be moved in another package. I am using discord.js and discord.js-commando and honestly I have no need for voice and stuff like that in my first few simple bots.
I mean no offence when I say this: If people can't npm i discord.js --no-optional --save then maybe it's a viable choice to have:

npm i -S discord.js
npm i -S discord.js-extra

I understand this package is used by many people - that doesn't mean this issue isn't real. I don't want to change the "core values" of this package but it is also very difficult for me to figure them out in light of this discussion.
What I want to say is I'd rather work on the solution (assuming you all agree what the requirements are) than work on the discussion of why this is a problem - it is a problem because NPM and this is a NPM module.

Please assert and communicate what the required parameters of the fix are and I will follow through

[0-vortex]

In favour of new requirements I moved this over to #1222