sc-redis icon indicating copy to clipboard operation
sc-redis copied to clipboard

Consider options rename

Open austinkelleher opened this issue 7 years ago • 3 comments

Problem

I think that the option names for Redis host and port should be renamed to be more Redis specific. When looking at the configuration in your SocketCluster project, it is unclear what the host and port properties are used for.

Proposal

Old:

const socketCluster = new SocketCluster({
  brokerOptions: {
    host: '54.204.147.15',
    port: 6379
  }
});

New:

const socketCluster = new SocketCluster({
  brokerOptions: {
    redis: {
      host: '54.204.147.15',
      port: 6379
      // Also supports easily passing additional options in the future
    }
  }
});

@jondubois I would be happy to work on this change if we decide this is the right direction.

austinkelleher avatar Feb 12 '18 14:02 austinkelleher

It looks a bit redundant, but I totally agree that's how it should be. + it's possible to implement with no breaking changes.

MegaGM avatar Feb 12 '18 18:02 MegaGM

@MegaGM It doesn't really matter that it's possible to make this a non-breaking change. Since this module is pre-1.0.0 any new published version may contain breaking changes since npm will lock it down anyway. In my opinion, we should not support both and drop support for the old way.

austinkelleher avatar Feb 12 '18 18:02 austinkelleher

@austinkelleher Sounds great. Feel free to go ahead. I think it's OK as a breaking change if we bump the major version to 1.0.0. So long as the snippets in this repo's README are also updated :)

jondubois avatar Feb 12 '18 19:02 jondubois