-
-
Notifications
You must be signed in to change notification settings - Fork 36
Added transaction helper #24
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
Conversation
ae82f20 to
5ee6545
Compare
5ee6545 to
fbf5a5a
Compare
index.js
Outdated
| const fp = require('fastify-plugin') | ||
| var pg = require('pg') | ||
|
|
||
| function transactionHelper (query, values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you support a double callback and promise interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean something like:
function transactionHelper (query, values, cb = null)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
index.js
Outdated
| client.query('COMMIT', (err) => { | ||
| done() | ||
| if (err) { | ||
| reject(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should return after rejecting.
index.js
Outdated
| client.query('BEGIN', (err) => { | ||
| if (shouldAbort(err)) reject(err) | ||
| client.query(query, values, (err, res) => { | ||
| if (shouldAbort(err)) reject(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should return after rjecting
index.js
Outdated
| var pg = require('pg') | ||
|
|
||
| function transactionHelper (query, values, cb = null) { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should not return a promise if there is a callback. Just use the callback version and if there is no callback return a promise.
if (!cb) {
return new Promise((resolve, reject) => {
this.transact(query, values, function (err, res) {
if (err) { return reject(err) }
return resolve(res)
})
})
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please have another look?
Sorry, I lack experience with promises and callbacks. Another issue is that I am having trouble building the project since I am on windows so just using here to run the tests 😄 It's a walk in the dark.
Apologies for spamming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some docs in the README as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
e2235ef to
80761b5
Compare
c3b59ec to
2b674e3
Compare
index.js
Outdated
| client.query('BEGIN', (err) => { | ||
| if (shouldAbort(err)) return cb(err) | ||
|
|
||
| fn(client).then(res => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pass a callback as well and support the dual promise/callback pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry I thought over this but am still confused. What will that callback look like? How do I know the user wants a callback?Could you please give me some pointers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example (from avvio) https://github.com/mcollina/avvio/blob/9f947aa5a71db44f185dfab16fdf75709d00dcb3/plugin.js#L82
120acc4 to
3708205
Compare
|
I know the last change is wrong but I am close to figuring it out 😄 |
Why you say so? Looks ok to me. |
|
I made it but then the usage didn't make sense to me. If I am understanding correct, will it look like: fastify.pg.transact((client, commit) => {
const id = client.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['brianc']);
commit(null, id)I am too used to |
|
It should be: fastify.pg.transact((client, commit) => {
client.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['brianc'], commit);
})or fastify.pg.transact((client, commit) => {
client.query('INSERT INTO users(username) VALUES($1) RETURNING id', ['brianc'], (err, id) => {
commit(err, id)
});
}) |
|
Ahh ok now it makes sense! I was thinking in terms of async usage mixed together with callback usage 😄 Thank you for taking your time on this and bearing with me. I will add a test case for that as well |
|
Thank you for working on this. I wanted to add this for a long time! |
3d4f833 to
c6e5516
Compare
c6e5516 to
dc6540a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits. This is good work!
7e66221 to
27ea9cf
Compare
27ea9cf to
f45b1a6
Compare
|
Should this be merged now @mcollina ? :) |
|
Yes! |
Addresses #5 and #26
Based on this example