Skip to content

Add Pool connection event, which fires when the Pool has created a new connection #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 14, 2013

Conversation

marcuswestin
Copy link
Contributor

This allows for e.g. setting Session variables on the connection before it gets used

…new connection. This allows for e.g. setting Session variables on the connection before it gets used
@felixge
Copy link
Collaborator

felixge commented May 14, 2013

Thx. LGTM. Feel free to merge.

marcuswestin added a commit that referenced this pull request May 14, 2013
Add Pool `connection` event, which fires when the Pool has created a new connection
@marcuswestin marcuswestin merged commit d536023 into master May 14, 2013
@marcuswestin marcuswestin deleted the pool-connection-event branch May 14, 2013 15:16
@dougwilson
Copy link
Member

@marcuswestin I just noticed this, but your connection event emits two arguments: err and connection. This seems weird, as I don't know any event signature in node that looks like that. IMO the event should only emit connection.

this.emit('connection', connection); 

It would be nice if you changed it before release (: If in the future it is ever wanted to emit the error connecting, that would be what the error event if for (or even a connectionError event if you want it to be ignorable/specific).

@marcuswestin
Copy link
Contributor Author

Good point, will change.
-- while mobile

On Tue, May 14, 2013 at 8:53 AM, Douglas Christopher Wilson
[email protected] wrote:

@marcuswestin I just noticed this, but your connection event emits two arguments: err and connection. This seems weird, as I don't know any event signature in node that looks like that. IMO the event should only emit connection.

this.emit('connection', connection); 

It would be nice if you changed it before release (: If in the future it is ever wanted to emit the error connecting, that would be what the error event if for (or even a connectionError event if you want it to be ignorable/specific).

Reply to this email directly or view it on GitHub:
#486 (comment)

marcuswestin added a commit that referenced this pull request May 14, 2013
@felixge
Copy link
Collaborator

felixge commented May 15, 2013

@dougwilson great catch, didn't spot this! Thanks.

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

Successfully merging this pull request may close these issues.

3 participants