Added transaction helper #24

cemremengu · 2018-09-20T22:44:20Z

Addresses #5 and #26

mcollina · 2018-09-21T09:59:25Z

index.js

@@ -3,6 +3,39 @@
 const fp = require('fastify-plugin')
 var pg = require('pg')

+function transactionHelper (query, values) {


Can you support a double callback and promise interface?

You mean something like:

function transactionHelper (query, values, cb = null)

mcollina · 2018-09-21T10:00:28Z

index.js

+          client.query('COMMIT', (err) => {
+            done()
+            if (err) {
+              reject(err)


you should return after rejecting.

mcollina · 2018-09-21T10:00:45Z

index.js

+      client.query('BEGIN', (err) => {
+        if (shouldAbort(err)) reject(err)
+        client.query(query, values, (err, res) => {
+          if (shouldAbort(err)) reject(err)


you should return after rjecting

index.js

mcollina · 2018-09-21T10:22:24Z

index.js

@@ -3,6 +3,38 @@
 const fp = require('fastify-plugin')
 var pg = require('pg')

+function transactionHelper (query, values, cb = null) {
+  return new Promise((resolve, reject) => {


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) }) }) }

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

index.js

mcollina

Can you add some docs in the README as well?

mcollina

LGTM

mcollina · 2018-09-22T07:01:55Z

index.js

+    client.query('BEGIN', (err) => {
+      if (shouldAbort(err)) return cb(err)
+
+      fn(client).then(res => {


Can you pass a callback as well and support the dual promise/callback pattern?

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?

Here is an example (from avvio) https://github.com/mcollina/avvio/blob/9f947aa5a71db44f185dfab16fdf75709d00dcb3/plugin.js#L82

cemremengu · 2018-09-24T07:04:06Z

I know the last change is wrong but I am close to figuring it out 😄

mcollina · 2018-09-24T07:14:06Z

I know the last change is wrong but I am close to figuring it out 😄

Why you say so? Looks ok to me.

cemremengu · 2018-09-24T07:24:53Z

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 async/await usage so I am having a hard time getting my head around this 😄

mcollina · 2018-09-24T07:26:42Z

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)
  });
})

cemremengu · 2018-09-24T07:30:23Z

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

mcollina · 2018-09-24T08:07:54Z

Thank you for working on this. I wanted to add this for a long time!

mcollina

A couple of nits. This is good work!

README.md

test.js

temsa · 2018-10-04T09:08:42Z

Should this be merged now @mcollina ? :)

mcollina · 2018-10-04T09:39:17Z

Yes!

cemremengu · 2018-10-04T09:42:26Z

This concludes #5 and #26 🎉

cemremengu force-pushed the transaction-helper branch 7 times, most recently from ae82f20 to 5ee6545 Compare September 20, 2018 23:54

Added transaction helper

fbf5a5a

cemremengu force-pushed the transaction-helper branch from 5ee6545 to fbf5a5a Compare September 20, 2018 23:57

mcollina reviewed Sep 21, 2018

View reviewed changes

Added callback support, return after rejections

e01991c

mcollina reviewed Sep 21, 2018

View reviewed changes

index.js Outdated Show resolved Hide resolved

cemremengu added 2 commits September 21, 2018 18:44

Separate promise and cb

66eee07

Added callback test

d7d188a

mcollina reviewed Sep 21, 2018

View reviewed changes

Updated readme with example

4db4943

mcollina approved these changes Sep 21, 2018

View reviewed changes

cemremengu mentioned this pull request Sep 21, 2018

Add capability to run multiple queries in a transaction. #26

Closed

Add multiple queries in transaction

80761b5

cemremengu force-pushed the transaction-helper branch from e2235ef to 80761b5 Compare September 21, 2018 20:24

Update README

2b674e3

cemremengu force-pushed the transaction-helper branch from c3b59ec to 2b674e3 Compare September 21, 2018 20:32

mcollina reviewed Sep 22, 2018

View reviewed changes

Added cb to function param

3708205

cemremengu force-pushed the transaction-helper branch from 120acc4 to 3708205 Compare September 23, 2018 20:08

cemremengu force-pushed the transaction-helper branch 2 times, most recently from 3d4f833 to c6e5516 Compare September 24, 2018 23:01

Add commit callback test, cleanup tests

dc6540a

cemremengu force-pushed the transaction-helper branch from c6e5516 to dc6540a Compare September 24, 2018 23:03

mcollina reviewed Sep 25, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

test.js Outdated Show resolved Hide resolved

cemremengu force-pushed the transaction-helper branch from 7e66221 to 27ea9cf Compare September 25, 2018 09:44

teardown tests, update README

f45b1a6

cemremengu force-pushed the transaction-helper branch from 27ea9cf to f45b1a6 Compare September 25, 2018 09:48

This was referenced Sep 25, 2018

Add Transaction Helper cemremengu/fastify-oracle#15

Closed

Add transaction helper fastify/fastify-mysql#13

Closed

mcollina merged commit d2158b7 into fastify:master Oct 4, 2018

cemremengu deleted the transaction-helper branch October 4, 2018 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added transaction helper #24

Added transaction helper #24

cemremengu commented Sep 20, 2018 •

edited

Loading

mcollina Sep 21, 2018

cemremengu Sep 21, 2018

mcollina Sep 21, 2018

mcollina Sep 21, 2018

mcollina Sep 21, 2018

mcollina Sep 21, 2018

cemremengu Sep 21, 2018 •

edited

Loading

mcollina left a comment

mcollina left a comment

mcollina Sep 22, 2018

cemremengu Sep 23, 2018 •

edited

Loading

mcollina Sep 23, 2018

cemremengu commented Sep 24, 2018

mcollina commented Sep 24, 2018

cemremengu commented Sep 24, 2018

mcollina commented Sep 24, 2018

cemremengu commented Sep 24, 2018

mcollina commented Sep 24, 2018

mcollina left a comment

temsa commented Oct 4, 2018

mcollina commented Oct 4, 2018

cemremengu commented Oct 4, 2018

Added transaction helper #24

Added transaction helper #24

Conversation

cemremengu commented Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemremengu Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemremengu Sep 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemremengu commented Sep 24, 2018

mcollina commented Sep 24, 2018

cemremengu commented Sep 24, 2018

mcollina commented Sep 24, 2018

cemremengu commented Sep 24, 2018

mcollina commented Sep 24, 2018

mcollina left a comment

Choose a reason for hiding this comment

temsa commented Oct 4, 2018

mcollina commented Oct 4, 2018

cemremengu commented Oct 4, 2018

cemremengu commented Sep 20, 2018 •

edited

Loading

cemremengu Sep 21, 2018 •

edited

Loading

cemremengu Sep 23, 2018 •

edited

Loading