refactor: use loop for transactions #875
Conversation
googlebot
added
the
cla: yes
schmidt-sebastian
force-pushed
the
mrschmidt/retry
branch
from
4453668 to
29f98ad
Compare
dev/src/transaction.ts
Outdated
| @@ -61,28 +62,17 @@ const READ_AFTER_WRITE_ERROR_MSG = | |||
| export class Transaction { | |||
| private _firestore: Firestore; | |||
| private _writeBatch: WriteBatch; | |||
| private _requestTag: string; | |||
| private _requestTag?: string; | |||
There was a problem hiding this comment.
It's not immediately obvious to me why this can't be passed into the constructor?
And, er, on that note, I can't figure out where this is ever assigned now? Seems like it'll always be undefined.
There was a problem hiding this comment.
It should have been assigned in runTransaction(). This does lead to a bunch of ? and ! in the code though, so I happily moved it back to the constructor.
Codecov Report
@@ Coverage Diff @@
## master #875 +/- ##
=========================================
+ Coverage 94.89% 96.6% +1.71%
=========================================
Files 25 25
Lines 15194 15544 +350
Branches 1136 1151 +15
=========================================
+ Hits 14418 15016 +598
+ Misses 767 519 -248
Partials 9 9
Continue to review full report at Codecov.
|
|
@thebrianchen Since this is pretty fragile code, do you mind looking at this as well? |
dev/test/write-batch.ts
Outdated
| @@ -356,6 +356,25 @@ describe('batch support', () => { | |||
| return promise; | |||
| }); | |||
|
|
|||
| it('cannot reset a committed batch', async () => { | |||
Our transaction retry is going to get more complicated (since we have to manually RETRY on aborted), so I thought it would be a good exercise to simplify the transaction runner.
This PR:
I was hoping there would be no behavior changes, but since we are now using an
asyncfunction, thetryblock aroundupdateFunction()now catches the "No read after write" error, which means that we will now rollback these transactions (which are empty).