Skip to content
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

pass through queries that are already objects #5

Merged
merged 1 commit into from
Apr 22, 2014
Merged

pass through queries that are already objects #5

merged 1 commit into from
Apr 22, 2014

Conversation

kamholz
Copy link
Contributor

@kamholz kamholz commented Apr 17, 2014

This closes #60 in any-db.

@grncdr
Copy link
Owner

grncdr commented Apr 21, 2014

Any chance you can take a look at the broken test?

@kamholz
Copy link
Contributor Author

kamholz commented Apr 21, 2014

I can reproduce the test failure, but I don't really understand it. It appears to fail when I revert the commit as well. I also haven't been able to reproduce it outside the test code.

@kamholz
Copy link
Contributor Author

kamholz commented Apr 22, 2014

OK, I think I've solved it now. node-any-db-adapter-spec calls conn.end() before the connection has had a chance to open. This causes pg.js to throw an error.

It looks like this could be solved by not calling conn.end() until the connection is open. But I'm not sure how to do that in a way that works for all adapters.

@grncdr
Copy link
Owner

grncdr commented Apr 22, 2014

Going a little deeper, the core issue is that this adapter was always passing pg.js a connect callback, even when the user didn't, which changed it's behaviour. I've made the necessary change over on #6, would you mind rebasing this PR off the latest master?

grncdr added a commit that referenced this pull request Apr 22, 2014
pass through queries that are already objects
@grncdr grncdr merged commit 829c2c7 into grncdr:master Apr 22, 2014
@grncdr
Copy link
Owner

grncdr commented Apr 22, 2014

Neat, turns out I can merge it without a rebase. Thanks for your contribution, a new version will be published soon 👍

@kamholz
Copy link
Contributor Author

kamholz commented Apr 26, 2014

When is "soon"? :-)

@grncdr
Copy link
Owner

grncdr commented Apr 26, 2014

published now

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

Successfully merging this pull request may close these issues.

2 participants