Have client.connect() return a Promise<RedisClient> #2602
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #2602 +/- ##
==========================================
- Coverage 95.70% 95.63% -0.07%
==========================================
Files 456 456
Lines 4561 4564 +3
Branches 524 526 +2
==========================================
Hits 4365 4365
- Misses 127 128 +1
- Partials 69 71 +2
☔ View full report in Codecov by Sentry. |
|
@franciscop thanks for contributing! I made some changes, wanna review them? 🙏 |
packages/client/lib/client/index.ts
Outdated
| async connect() { | ||
| // see comment in constructor | ||
| this.#isolationPool ??= this.#initiateIsolationPool(); | ||
| await this.#socket.connect(); | ||
| return this; | ||
| return this as unknown as RedisClientType<M, F, S>; | ||
| } |
There was a problem hiding this comment.
Looks good! I am not familiar enough with TS to know if there's any meaningful difference here, but I've seen that style in the past so sounds good.
| testUtils.testWithClient('connect should return the clietn instance', async client => { | ||
| try { | ||
| assert.equal(await client.connect(), client); | ||
| } finally { | ||
| if (client.isOpen) await client.disconnect(); | ||
| } |
There was a problem hiding this comment.
much cleaner test, thanks for the update!
|
wow it's a honor to see the changes be the new main recommended way to connect to Redis! I went and submitted these changes as an alternative mode only, but yeah happy to see them up front! For the new code: the types seem good, thanks for correcting them I'm not that familiar with TS. Same for the test, your update is much cleaner! Unfortunately I cannot approve the PR since I'm not part of node-redis, but yes they all seem good! |
Co-authored-by: Francisco Presencia <[email protected]>
|
I'll merge it and release it "soon" (along with a few more PRs) 🎉 |
|
@franciscop |
|
Thanks!! 🥳🥳 |
|
How about |
|
@leppaott in v5.. :) |
Description
Make the
.connect()method to return a promise with an instance of itself, so we can do:Issue: Have client.connect() return a
Promise<client>instead ofPromise<void>As explained above, added a case that simplifies the creation of the client for those who want to initialize AND connect at the same time, while retaining the ability to do so separately as before otherwise.
Added a test to make sure the return of
connet()is the same client instance. I noticed there was no test explicitly for.connect(), so created it under thedescribe('.connect()')name.Checklist
npm testpass with this change (including linting)? (tests)