Add onConnect callback which allows "setup" of a pooled client#3620
Add onConnect callback which allows "setup" of a pooled client#3620
onConnect callback which allows "setup" of a pooled client#3620Conversation
Deploying node-postgres with
|
| Latest commit: |
ca8c336
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a97607d8.node-postgres.pages.dev |
| Branch Preview URL: | https://bmc-pool-callbacks.node-postgres.pages.dev |
| maxLifetimeTimeout.unref() | ||
| client.once('end', () => clearTimeout(maxLifetimeTimeout)) | ||
| if (this.options.onConnect) { | ||
| let hookResult |
There was a problem hiding this comment.
How about a single promiseTry(this.options.onConnect).then(…, …) path? Where
const promiseTry = Promise.try ?? (f) => new Promise((resolve) => {
resolve(f())
})There was a problem hiding this comment.
I guess this gets a little more complicated if it has to support the custom Promise config option.
There was a problem hiding this comment.
(Note: I’m bringing this up ahead of publishing instead of making a follow-up PR because the behaviour is slightly different, so it would be a breaking change.)
There was a problem hiding this comment.
much appreciated! Though I'm not sure what you mean by a breaking change...what's a breaking change, exactly?
There was a problem hiding this comment.
The promiseTry suggestion, compared to what’s currently in this PR. They’re slightly different from each other, so it couldn’t wait until afterwards, I mean.
2 participants
This allows you to do
for example.
If the
onConnectfunction throws or rejects the error will be sent out to the callsite ofpool.connectorpool.query& the client will be disposed of. This only triggers when the client is first connected to postgres not when an existing connected client is checked out.Why did I call it
pool.connectso long ago instead ofpool.getClient()orpool.aquire()or something? I don't know. All I know is I'm not very fond of myself from 10 years ago. Oh well! I'll make sure to include good documentation on this change before I actually merge it but want to put it up for some 👀This should fix #3617
I omitted the other hook ideas I had as I need to think through them more, and in the case of
onClosethe release method is sync, and some times clients are closed in the background so error handling there is a different issue I'll need to think about.