Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

peerDependencies to optionalDependencies (package.json) #280

Merged
merged 1 commit into from
Sep 2, 2019

Conversation

knoxcard
Copy link
Contributor

@knoxcard knoxcard commented Aug 30, 2019

optionalDependencies dont spill a bunch of warnings on the screen. I use ioredis and it's reminding me to always install redis and redis-mock. every time I run yarn upgrade --latest.

optionalDependencies
Optional dependencies are just that: optional. If they fail to install, Yarn will still say the install process was successful.This is useful for dependencies that won’t necessarily work on every machine and you have a fallback plan in case they are not installed (e.g. Watchman).
https://yarnpkg.com/lang/en/docs/dependency-types/

optionalDependencies dont spill a bunch of warnings on the screen. I use ioredis and it's reminding me to always install redis and redis-mock.

https://yarnpkg.com/lang/en/docs/dependency-types/

optionalDependencies
Optional dependencies are just that: optional. If they fail to install, Yarn will still say the install process was successful.

This is useful for dependencies that won’t necessarily work on every machine and you have a fallback plan in case they are not installed (e.g. Watchman).
@knoxcard knoxcard changed the title Change peerDependencies to optionalDependencies peerDependencies to optionalDependencies (package.json) Aug 31, 2019
@wavded wavded merged commit 008fc43 into tj:master Sep 2, 2019
@wavded
Copy link
Collaborator

wavded commented Sep 2, 2019

Yep, that makes sense to me, I suppose this library technically doesn't require any dependencies, but won't do a whole lot with a Redis client 🤣

@silentroach
Copy link

silentroach commented Sep 3, 2019

With peerDependencies npm will not install ioredis and redis and redis-mock, but with optionalDependencies it will.
And it is not great though. Why should I have ioredis package in my node modules if I use redis client?

you have a fallback plan in case they are not installed

What fallback do you mean? Connect-redis will not work if I will not provide any client in init function.

@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 3, 2019

@silentroach - why a thumbs down??
You can provide either a redis or ioredis client.

Before this module would default to redis. Lets say I wanted to use ioredis instead. I would have to keep the redis package installed in my repository. Now, you can just keep the ioredis package installed and remove the redis package. Only one client required, no extra fat!

@silentroach
Copy link

I know, but I don't want them both to be installed

@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 3, 2019

I know, but I don't want them both to be installed

Only install the redis client you're going to use. If another one is also installed, remove it. Basically, you should have redis or ioredis.

@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 3, 2019

@silentroach - wait, this just hit me, lol. ummm, dependencies should be completely removed. Is that what you meant? lol, time for another pull request!

@silentroach
Copy link

silentroach commented Sep 3, 2019

They both are installed now with connect-redis because of they are optional dependencies.

Screen Shot 2019-09-03 at 10 55 40

With peerDependencies I can install only the client I need:

Screen Shot 2019-09-03 at 10 59 27

@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 3, 2019

Crap! okay.
So I've made a terrible mistake, we need to change it back to how it was?

What about removing dependencies altogether?

@wavded - oopsy, looks like I made a booboo!

@silentroach
Copy link

silentroach commented Sep 3, 2019

I think peerDependencies were ok :}

@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 3, 2019

Sure, right now that sounds like the better option.
But what would happen, if we just drop dependencies altogether?

069497c

@silentroach
Copy link

silentroach commented Sep 3, 2019

peerDependencies are something like a contract, it is very useful, but it is better to think about it not like a dependency to some package, but to some version of package. If connect-redis will work with all versions of that packages, you just don't need to describe them in dependencies.

@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 3, 2019

But at the same time, we're moving forward in time, who's still using super old dependencies? I think it's better those who do receive a console error.

@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 3, 2019

@wavded - seems that I completely misunderstood how optionalDependencies works. Currently, when users install connect-redis, all defined packages, in this case three (ioredis, redis, redis-mock) are included as well. :-(

Even setting peerDependencies reminds the user of these three packages, every time they install or upgrade, which also appears to be incorrect, right?

Would the best option be to simply remove dependencies?
#282

@knoxcard knoxcard mentioned this pull request Sep 3, 2019
@wavded
Copy link
Collaborator

wavded commented Sep 3, 2019

Thanks for the report @silentroach, yes @knoxcard seems the best course of action is to remove it from the dependencies all together. Of course it will exist in devDependencies for testing.

@knoxcard knoxcard deleted the patch-8 branch September 4, 2019 12:22
@knoxcard
Copy link
Contributor Author

knoxcard commented Sep 4, 2019

Yes, major thanks to @silentroach for reporting that! This repo is downloaded 130,000+ times per week, All of those unnecessary downloads would have continued, until someone else pointed that out.

jellis24 added a commit to jellis24/connect-redis that referenced this pull request Apr 6, 2022
optionalDependencies dont spill a bunch of warnings on the screen. I use ioredis and it's reminding me to always install redis and redis-mock.

https://yarnpkg.com/lang/en/docs/dependency-types/

optionalDependencies
Optional dependencies are just that: optional. If they fail to install, Yarn will still say the install process was successful.

This is useful for dependencies that won’t necessarily work on every machine and you have a fallback plan in case they are not installed (e.g. Watchman).

PR: tj/connect-redis#280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants