move promise-based API to separate file

Conflating promises & callbacks in the exact same API was a mistake. 😦 I'm going to move the promise based versions of client.query & client.connect (and any other overloaded method) out into a separate file within pg. I still like shipping a slim version with the repo, because it helps me keep everything in lock-step and handle some of the weird edge-cases with tests, but they need to be separated into their own API surface area and only included if the user specifically does something like require('pg/promise') or something - haven't figured out the API details yet.

I'm also going to give a bit of a look at pg-pool and see which methods we might need to de-promisify or split out into a promise & non-promise version.

tl;dr - callback + promise api on the same method = bad news bears.

tl;dr - callback + promise api on the same method = bad news bears.

Yes we had the same issue on our JavaScript client at Algolia.

Nowadays our plan is to ship only a promise based solution and delegate callback based to another package (require('algoliasearch-cb')) OR to user land (callbackify(algoliasearch)).

The reason for this is that we believe the promise based flow control AND the introduction of the async/await keywords (that are available today in many many environment) is a better async flow control mechanism than callback based.

Was my 2 cents :D

@vvo

Thanks for the input @vvo! Yeah - I really like the async/await stuff too & use it in all my own stuff & client work which is what prompted me to put in promises initially. btw: aloglia is pretty sweet - do y'all use node-postgres there?

@charmander

@charmander @joskuijpers

Howdy! Pinging y'all since y'all have both mentioned the promise thing before. I wanted to discuss briefly how to fix the promises API. Within node-postgres there is only one place where a promise is used - the Query returned from a call to client.query implements the promise api, though it's not exactly a promise itself. I should have stopped and thought more before doing that in v6.0 because it's caused some issues, and overloading the client.query function even further was just a bad move. So I have a few different ideas on how to handle this going forward...any of these changes are going to break backwards compatibility so it'll be in 7.0:

change client.query back to how it was and add a client.queryAsync() method that returns a promise instead of a query object. That's pretty much what you'd get if you just hit the library with Bluebird.promisify(client) I think. Straight forward. The only thing is I've always felt having the Async suffix on a function that returns a promise is a touch gross considering things in node are async by default.

const { rows } = await client.queryAsync('select * from foo where name = $1', 'bar')

change client.query back to returning a query object, and add an explicit .promise() method onto the query object and that returns a promise for the result of the query and suppresses the error event that would be emitted from the query and instead rejects the promise in that case:

const { rows } = await client.query('seleect * from foo where name = $1', 'bar').promise()

do something w/ require paths so if you want to use promises you require something different and get a promise based api that mirrors the callback api:

const Client = require('pg/client/promise')
const client = new Client()
await client.connect()
const { rows } = await client.query()
await client.end()

That'd make keeping the two APIs separate, which is probably nice from a maintenance perspective. It would make mixing callback & promises in a client library a pain which I could see being really frustrating.

Whatever we decide for this method I'll likely take a similar approach to the methods in pg-pool.

Totally open to other ideas y'all have too. What do y'all think?

@brianc

@brianc Since handling the 'row' event isn’t particularly common, I’d be for having query return a promise and creating a separate queryStreaming (with a better name) that emits events. Understandable if that’s too much of an API change, though.

@charmander

+1 for having the core API return a Promise. That would allow for continuing to support callbacks via the same interface as well. And while I'm not a big fan of having overloaded functions like that (, it does have the advantage of being least intrusive from a backwards compatibility perspective.

Per @charmander's point, usage of the event API is dwarfed by Promise/callback based usage so the latter should be the first class citizen. I'd imagine it'll be even more so with async/await growing in usage.

Cool! Thanks for the feedback! I agree - the event API is extra weird IMO because it encourages using the query as a type of stream, except the query doesn't support pause, reads events as fast as the possible come in no matter what, and in the case of the native bindings can in certain cases even buffer the entire result-set into C in libuv before sending any rows to node. It encourages trying to prematurely optimize "streaming" results, when really if you want to stream results you should use pg-query-stream.

So what about this:

callback mode can still return the query object mostly to not
cause as much backwards compatibility breakage

const query = client.query('SELECT * FROM foo', (err, res) => {

})

// this still works, for backwards compat
query.on('row')

if you don't provide a callback we'll return a promise not a query object

const promise = client.query('SELECT * FROM foo')
promise.on('row') // throws: query doesn't have an .on function
promise.then(res => console.log(res.rows)) // this works

We could then return the object passed to the query method if the object implements a certain interface: this would allow pg-query-stream, pg-cursor and a few other libs continue to function. And if you really want to get the old-school query object returned so you can do the query.on('row') stuff - you instantiate one and pass it yourself:

const query = new Query({
  text: 'SELECT * FROM foo'
})

const q = client.query(query)
q == query // true
q.on('row', (row) => console.log('this works'))

What you think?

I think this just about hits the sweet spot between sane and backwards compatible.

+1 to maintaining the original callback handling.
+1 to maintaining promises as first class citizens.
+1 to passing back the original object for "fancy" types (ex: Query)
-1 to adding the .on(...) handlers if a callback is specified. Maybe I'm missing something, but in what situation would someone use both the .on('row', ...) and include a callback to process the entire result?

Requiring callbacks be specified to get event style handling (with regular strings vs. special objects) is already going to break anyone using that feature. If we're going to make a breaking change, let's clean it up once and for all. For the last one how about adding something like OldSchoolQuery(...) (most likely with a better name...)? That way anyone using the .on(...) handlers could switch to that? Then the regular Query wouldn't need to carry that baggage forward and it could even live in a separate repo.

Initial work on this has been done here

Still to do:

clean up promsification a bit, I did it sloppy at my first pass.
add deprecation warning to client.query('SELECT *').on('row') family of event emitters - don't just remove them and shatter backwards compatibility without a deprecation minor version release. I feel like it's too jarring...I know new-relic and others depend on the event emitter in situations so I want to understand those use-cases and avoid causing production problems or upgrading headaches for thousands of developers without a deprecation release cycle.
document upgrade path for using client.query(new Query('SELECT *')).on('row') if you absolutely need row emitter for some reason. Also document the reasons not to use event emitter. Briefly outlined here among other places.

clean up promsification a bit, I did it sloppy at my first pass.

Cool. Will review some time this week.

add deprecation warning to client.query('SELECT *').on('row') family of event emitters - ... don't just remove them and shatter backwards compatibility without a deprecation major version release.

So are you saying you'll be pushing a 6.x release that includes the deprecation notices and PR #1323 (Promisfy) will become semver major 7.0?

@sehrope

Yah @sehrope - put in a PR to deprecate and warn on things from [email protected]. #1327

Starting to get excited for the quality improvements in 7.0!

Nice. I noticed the new site too ... snazzy!

Thanks!

Okay this is done! 7.0 has the methods deleted & 6.x has the deprecation PR ready to merge!

Navigation Menu

Repository navigation

move promise-based API to separate file #1299

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

move promise-based API to separate file #1299

Description

Activity

vvo commented on May 24, 2017

brianc commented on May 24, 2017

brianc commented on May 26, 2017

charmander commented on May 29, 2017

sehrope commented on May 29, 2017

brianc commented on May 29, 2017

sehrope commented on Jun 2, 2017

brianc commented on Jun 15, 2017

sehrope commented on Jun 15, 2017

brianc commented on Jun 18, 2017

sehrope commented on Jun 18, 2017

brianc commented on Jun 18, 2017

brianc commented on Jun 18, 2017

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions