-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add transactor.parentTransaction #5567
Conversation
629e994
to
acc28e5
Compare
acc28e5
to
fd8da92
Compare
Any feedback here? |
fd8da92
to
8365d76
Compare
@OlivierCavadenti would you mind taking a look? |
8365d76
to
fbf5d8f
Compare
fbf5d8f
to
2746010
Compare
@OlivierCavadenti friendly ping |
2746010
to
9830235
Compare
@kibertoad what if instead of having another property for the top transaction promise we would have one of those to allow full traversal of the transactions:
I prefer option 2 as it's:
|
@rluvaton yeah, I agree! |
4d61bb7
to
9ca65c4
Compare
Fine with me. I've updated the implementation. |
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.
Great job!
@@ -219,6 +219,10 @@ class Transaction extends EventEmitter { | |||
return makeTransactor(this, connection, trxClient); | |||
}) | |||
.then((transactor) => { | |||
this.transactor = transactor; |
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.
executionPromise is not available on outerTx
directly. Need a reference to it somewhere.
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.
Thanks, yeah after I wrote I saw
@@ -219,6 +219,10 @@ class Transaction extends EventEmitter { | |||
return makeTransactor(this, connection, trxClient); | |||
}) | |||
.then((transactor) => { | |||
this.transactor = transactor; | |||
if (this.outerTx) { | |||
transactor.parentTransaction = this.outerTx.transactor; |
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.
Please define this and the transactor property in the constructor (with default undefined value) to take advantage of v8 hidden classes
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.
This seems unprecedented? For example context
, isTransaction
, userParams
, savepoint
, commit
, rollback
, isCompleted
, and executionPromise
are all not defined on the QueryBuilder
constructor.
@@ -219,6 +219,10 @@ class Transaction extends EventEmitter { | |||
return makeTransactor(this, connection, trxClient); | |||
}) | |||
.then((transactor) => { | |||
this.transactor = transactor; |
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.
executionPromise is not available on outerTx
directly. Need a reference to it somewhere.
@kibertoad LGTM |
@caseywebdev I assume TS types need to be updated as well? |
9ca65c4
to
7c318f7
Compare
Just pushed |
Thank you! Can you also add a note about this feature to https://github.com/knex/documentation? |
Released in 3.1.0 |
Recently our app has had the need to know when the base transaction has committed, regardless of nested transaction levels. This works with
executionPromise
when you're one transaction deep, but if you have nested transactions theexecutionPromise
only refers to the inner-most transaction. This change adds abaseExecutionPromise
property on the transactor that will always reference the outer-mostexecutionPromise
.Our specific use case is that we have functions that receive transactions and may open up nested transactions which can open up more nested transactions. Sometimes in those transactions, offline work records are inserted into a queue table to be processed out-of-band of the current transaction. We want to start that work as soon as the transaction is committed, but if we attempt to process the work when
executionPromise
is resolved in a nested transaction, the outer transaction is still not committed and the records aren't visible to the work processors yet, so no work is consumed.I've monkey-patched this for our purposes for now, but I think it's a valuable addition to the core. For anyone interested in the patch: