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

Query event in Transaction should be proxied to the main client #984

Closed

Conversation

wubzz
Copy link
Member

@wubzz wubzz commented Sep 19, 2015

As discussed with @tgriesser in #855 the query event is no longer triggered onto the main client when in a transaction. It was simply a miss during the transaction overhaul, according to tgriessers's comments.

Example code:

dbClient.on('query', function(obj) {
    console.log('onQuery'); //Works in knex < 8.0.0, fails in >= 8.0.0
    return obj;
});

dbClient.transaction(function(tr) {
    return tr.raw('SELECT version()').then(tr.commit).catch(tr.rollback);
}).then(_.noop).catch(_.noop);

There may be better ways of fixing this, I don't know.

@wubzz wubzz changed the title Query event should be proxied to the main client Query event in Transaction should be proxied to the main client Sep 19, 2015
@rhys-vdw
Copy link
Member

Hi @wubzz. That looks reasonable to me, and looks like it closes #855.

I've made a rule not to merge anything without tests as I am also not that familiarized with the codebase. So please add a couple of tests for these events and I'll merge it in.

@wubzz
Copy link
Member Author

wubzz commented Sep 20, 2015

@rhys-vdw Truthfully having issues installing dependencies for knex on my windows env, and can't get it to work. Seems to primarily be mariasql where I have issues..

I commited a test hoping that I added it in the right place (also lack experience with the project structure/codebase), and hoping CI will pull through.

Pretty straight-forward test, tho.

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