Skip to content
This repository has been archived by the owner on Dec 30, 2019. It is now read-only.

Add connectionSetup as configuration option to prepare a client #43

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bramkoot
Copy link

@bramkoot bramkoot commented Jan 26, 2017

I propose to add an option 'connectionSetup' to make it possible to do setup before a client is used. My own use-case is to set the search path to a certain schema.

This relates to an open issue at node-postgres about the same use case: brianc/node-postgres#1123

I use node-postgres in combination with pg-async and with the adjustment you would be able to do something like this:

const connection = new PgAsync({
  connectionString, 
  connectionSetup: (client, cb) => {
    client.query('SET search_path TO users')
       .then(_args => {
         cb(null, client)
       }).catch(cb)
  }
})

@brianc
Copy link
Owner

brianc commented Mar 9, 2017

Yeah i really dig this big time.

@brianc
Copy link
Owner

brianc commented Mar 9, 2017

Sorry I just saw it! :p been buuuusy! (too busy!)

If you write some tests for this I can merge it no problemo.

index.js Outdated
@@ -72,7 +72,12 @@ Pool.prototype._create = function (cb) {
} else {
this.log('client connected')
this.emit('connect', client)
cb(null, client)

if (this.options.connectionSetup && typeof this.options.connectionSetup === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions can’t be falsy, so this just needs the typeof check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… although it probably shouldn’t fail silently, so maybe just if (this.options.connectionSetup !== undefined).

@bramkoot
Copy link
Author

bramkoot commented May 4, 2017

I've added some simple tests which works fine, but Travis fails for older node versions with some odd errors. Any idea's what that could be?

node 0.10 / 0.12:

standard: Unexpected linter output:
TypeError: Failed to load plugin react: Object function Object() { [native code] } has no method 'assign'

@charmander
Copy link
Collaborator

The linter isn’t compatible with Node ≤0.12 anymore. (It would be nice if pg didn’t have to be either…)

@brianc
Copy link
Owner

brianc commented May 5, 2017

@charmander - I agree. In 1 week I'll be leaving full time employment to do freelancing and more full-time node-postgres support. At that point we should have a discussion about dropping old versions, splitting out promises into a sub-module of sorts, and doing other backwards incompatible changes. I want 7.x and beyond to be valuable and shed a bit of the support for versions of node that are > 3 years old.

@bramkoot
Copy link
Author

bramkoot commented May 5, 2017

Sounds good @brianc! Let me know if you need any help with this PR.

@brianc brianc mentioned this pull request Jul 9, 2017
Merged
@seriouscoderone
Copy link

@brianc Let's wrap this up. This would be useful.

var pool = new Pool({
host: 'no-postgres-server-here.com',
connectionSetup: function () {
called = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to call the callback to fail properly.

pool.query('SELECT $1::text as name', ['brianc'], function (err, res) {
expect(err).to.be.an(Error)
expect(called).to.be(false)
pool.end(function (err) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pool.end(done)

it('does not call setupConnection when an error occurs', function (done) {
var called = false
var pool = new Pool({
host: 'no-postgres-server-here.com',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe start a net.Server on a random port and use it as the host instead, to make the test fully local.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants