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

cache clientId on socket connection, to not require it to be awaited

Open sjpotter opened this issue 1 year ago • 2 comments

Description

Describe your pull request here

clientId will be removed from a redis command sent over the wire each time (and therefore need to be awaited) and instead cached. This moves it from the dynamic RedisClientType to the static RedisClient making it more usable for internal functionality.

I don't think this should break backwards compatibility as await on number should in effect be the same thing as await on Promise (but I might be wrong?)

Checklist

  • [ ] Does npm test pass with this change (including linting)?
  • [ ] Is the new or changed code fully tested?
  • [ ] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

sjpotter avatar Jan 15 '24 09:01 sjpotter

  1. it breaks backward compatibility for redis-server < 5.0.0 (the main README does say its the minimum supported version because its the minimum version we test with, but im not sure if we wanna fully break it from older versions).
  2. it adds some overhead for something most users don't need.

leibale avatar Jan 16 '24 18:01 leibale

Ok, I buy the < 5.0 as some sort of reasoning, but in terms of overhead, I think its actually something users "do" need. i.e. for watch tracking across reconnect, while it can be accomplished in other ways, this kills many birds with one stone without tracking other things.

sjpotter avatar Jan 16 '24 19:01 sjpotter