launchdarkly / js-core

LaunchDarkly monorepo for JavaScript SDKs
Other
12 stars 12 forks source link

LDRedisOptions typing bug #479

Closed mikewesthad closed 3 weeks ago

mikewesthad commented 3 weeks ago

Is this a support request? No

Describe the bug

The LDRedisOptions exported don't match the ioredis constructor options. Specifically, a redis:// string is an acceptable option to construct a ioredis instance. This is how we connect to redis when using LD so it would be ideal if this were supported and we didn't need to do a ts ignore error:

const featureStore = RedisFeatureStore({
  // @ts-expect-error LD's wrapper around ioredis doesn't have the correct types
  // - a string URL with username, password and port is acceptable.
  redisOpts: redisUrl,
  prefix: `launchdarkly:replit:${process.env.NODE_ENV}`,
  cacheTTL: 30,
});

To reproduce See above

Expected behavior Expect to be able to connect using the same options that ioredis supports

Logs N/A

SDK version

"@launchdarkly/node-server-sdk": "~8.2.6",
"@launchdarkly/node-server-sdk-redis": "^4.1.14",

Language version, developer tools Node 20

OS/platform Ubuntu

Additional context N/A

kinyoklion commented 3 weeks ago

Hello @mikewesthad,

We support the RedisOptions directly from ioredis, but we do not support the various short-hand constructor options.

Alternatively you can omit the redisOpts and directly construct a client and pass it to the client parameter.

Thank you, Ryan

mikewesthad commented 3 weeks ago

Ah, I see. We already have a redis client lib that isn't ioredis so a bit of a bummer to pull in a second client lib for this to make LD redis work w/o ts errors. Thanks!

kinyoklion commented 3 weeks ago

@mikewesthad Which redis library are you using?

Under the hood it currently required ioredis, so it may get encapsulated, but currently it will be there either way.

We had moved to ioredis because it was more capable and widely used than the library we had used previously. But if there are sufficient users and capabilities, then we could consider additional integrations in the future.

Thank you, Ryan