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

Remove redis from dependencies #262

Closed
daveisfera opened this issue Apr 16, 2019 · 10 comments
Closed

Remove redis from dependencies #262

daveisfera opened this issue Apr 16, 2019 · 10 comments

Comments

@daveisfera
Copy link

Currently, redis has to be installed even if it's not used. It shouldn't be included in dependencies so ioredis can be used without having to install redis

@knoxcard
Copy link
Contributor

knoxcard commented Apr 16, 2019

https://github.com/tj/connect-redis/blob/master/lib/connect-redis.js

var redis = require('redis')

    // convert to redis connect params
    if (options.client) {
      this.client = options.client;
    } else {
      var redis = require('redis');

      if (options.socket) {
        this.client = redis.createClient(options.socket, options);
      }
      else {
        this.client = redis.createClient(options);
      }
    }

Hmm, so it seems that redis is the default if nothing is specified. I see your point!

@knoxcard
Copy link
Contributor

@wavded - what should be done here?

@daveisfera
Copy link
Author

peerDependencies are a common way to deal with this, but I don't believe that they allow specifying "either or" types of relationships:
https://docs.npmjs.com/files/package.json#peerdependencies

@knoxcard
Copy link
Contributor

@daveisfera - peerDependencies, that looks like the solution to me right there!

@daveisfera
Copy link
Author

It's probably the best option right now, because peerDependencies would prevent redis from being installed when it's not needed, but would result in a warning that it's not installed until there's support for specifying an or for multiple packages to satisfy the requirement.

See the following:
https://npm.community/t/allow-any-one-of-specified-packages-in-peerdependencies/4933
duaraghav8/solium-plugin-security#33

@wavded
Copy link
Collaborator

wavded commented Apr 17, 2019

This would be a breaking change and require a major version update, seems pretty minor if its included but not used to me. We did recently update it to not include the files in the runtime if not used as well. The vast majority of people use the main redis package.

@daveisfera
Copy link
Author

I agree that it would be a breaking change, but it results in 4 packages being installed and that takes up almost 400KB, so it would be great to not have to include it when we use it.

@wavded wavded mentioned this issue Aug 26, 2019
Merged
1 task
@wavded
Copy link
Collaborator

wavded commented Aug 28, 2019

We no longer bundle a Redis client in V4.

@wavded wavded closed this as completed Aug 28, 2019
@daveisfera
Copy link
Author

Thanks!

@knoxcard
Copy link
Contributor

Sweeet!!!

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

No branches or pull requests

3 participants