feature: multiple connections #11
Conversation
There was a problem hiding this comment.
Nice work!
Just few nits.
I would make this configurable, if I don't pass a name option, I should be able to use fastify.pg.[api] without the name key.
Somethink like this should work:
const db = {
connect: pool.connect.bind(pool),
pool: pool,
Client: pg.Client,
query: pool.query.bind(pool)
}
if (name) {
fastify.pg[name] = db
} else {
fastify.pg = db
}What do you think?
index.js
Outdated
|
|
||
| fastify.addHook('onClose', onClose) | ||
| fastify.addHook('onClose', (fastify, done) => pool.end(done)) |
There was a problem hiding this comment.
Why this change?
The reason of the separated function is that if something goes wrong its easier understand which function has failed, since it has a name ;)
There was a problem hiding this comment.
-
I had tried
nameconfigurable first time, but I think maybe it makes some user confuesed. sometimespgis an connection container and sometimes it is an connection. I can not make a choice myself. If you thinknameconfiguable better, I could do it. -
using closure because I need refer the connection pool. If I still use
onClosefunction, I cloud not get the pool to end.
index.js
Outdated
| fastify.pg[name] = db | ||
| } else { | ||
| if (fastify.pg) { | ||
| console.warn('fastify-postgres has already registered') |
There was a problem hiding this comment.
we should error if it was already register.
test.js
Outdated
| connectionString: 'postgres://postgres@localhost/postgres' | ||
| }) | ||
|
|
||
| fastify.ready(err => { | ||
| t.error(err) | ||
| fastify.pg.connect(onConnect) | ||
| fastify.pg.test.connect(onConnect) |
There was a problem hiding this comment.
I would not change the tests, and keep using the non-prefixed version. But add a new test to check on the prefixed version.
|
LGTM. @delvedor do you want to have another look before releasing? |
This PR is for the feature multi-connections support(#10).