SocketCluster / sc-redis

Redis adapter for SocketCluster
45 stars 11 forks source link

Consider options rename #13

Open austinkelleher opened 6 years ago

austinkelleher commented 6 years ago

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.

MegaGM commented 6 years ago

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

austinkelleher commented 6 years ago

@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.

jondubois commented 6 years ago

@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 :)