Return the client instance in the connect() method

[franciscop]

Note: I open this PR as a conversation starter, I've explained my reasoning in a Redis Issue here. I'd be happy to open an issue instead, but I thought showing the code would be faster/easier.

I'd like to suggest returning the client instance in the .connect() method, so that we can create a client in a single step:

const client = await new Client().connect();

If this change is welcome, I'd be happy to add more things to this PR like testing, documentation, etc.

[brianc]

yeah that's a nice little API QOL improvement! Tests & docs & we're good to merge! Sorry for the delay!

[franciscop]

Thanks! I'll try to get it done over the weekend!

[brianc]

awesome! no rush...it's all appreciated. ❤️

[franciscop]

I think it's ready! I replaced the main docs bit for connect(), but please let me know if you prefer I add it as an "alternative" instead and keep that line like the original 🙇

[brianc]

Thanks @franciscop! One thing: can you change the native client as well to do the same behavior? The tests are currently failing now due to the difference.

[franciscop]

Thanks, fixed it. A couple of things I noticed:

1. This test was repeated in the file I'm modifying, I'd assume on purpose?

suite.test('valid connection completes promise', () => {
  const client = new pg.Client()
  return client.connect().then(() => {
    return client.end().then(() => {})
  })
})

2. I'm only modifying the await client.connect(), while leaving the callback-based client.connect(() => {}) as-is without the client parameter, I thought this would be okay since now most people use async/await but please let me know if you want me to pass the client param there as well.

[brianc]

2. I'm only modifying the await client.connect(), while leaving the callback-based client.connect(() => {}) as-is without the client parameter, I thought this would be okay since now most people use async/await but please let me know if you want me to pass the client param there as well.

I think for the pricipal of least surprise, it would probably be better to pass it to the callback. But yeah...I don't think its common to use the callback APIs anymore. I'm considering removing them in pg@9.x though last time I tried (several years ago) some folks got rather upset....so not sure yet. :)

[franciscop]

Now that I have a better look, it seems in fact that .connect() with callback was already passing the client instance to the function:

        this._connectionCallback(null, this)

But the native client was not, fixed it:

-      cb()
+      cb(null, this)

I'm considering removing them in pg@9.x though last time I tried (several years ago) some folks got rather upset....so not sure yet

I hear you, in my bubble it seems like long ago that people used callbacks instead of promises, but I've never maintained such a popular project like this so I don't know about the people using it too well.