Add onCommitted listeners for transactions #2265
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/listener
branch
from
fb2e838 to
62a306a
Compare
There was a problem hiding this comment.
It would be nice to see a tests that showed:
- When a transaction fails, the action isn't taken
- When a transaction retries, the action is taken only once, afterward
I think that's how this is going to work based on just reading this but writing tests has a funny habit of showing me that code I thought was obviously correct is actually subtly broken.
| abstract readonly currentSequenceNumber: ListenSequenceNumber; | ||
|
|
||
| addOnCommittedListener(listener: () => {}): void { |
There was a problem hiding this comment.
Shouldn't we require that the committed listeners return void? That way you can't accidentally have a return statement in your callback that you intend for the outer method.
Or would that interfere with using short arrow functions in the callback?
There was a problem hiding this comment.
I just realized this when I started to modify my "applyRemoteEvent" PR. It is much better if we return void.
| @@ -180,6 +180,7 @@ export class MemoryPersistence implements Persistence { | |||
| this.referenceDelegate.onTransactionStarted(); | |||
| return transactionOperation(txn) | |||
| .next(result => { | |||
| txn.raiseOnCommittedEvent(); | |||
There was a problem hiding this comment.
This ordering seems off. Couldn't the reference delegate conceivably modify the transaction?
IndexedDbPersistence has the better implementation. Chaining this off the promise makes it clearer that it's altogether outside the transaction.
There was a problem hiding this comment.
Changed this to invoke the committed handler as the last step.
|
Tests added. Whee. |
| ++commitCount; | ||
| }); | ||
|
|
||
| ++runCount; |
There was a problem hiding this comment.
Optional: you might also consider expecting that commitCount is zero during the transaction run.
* Add transaction retries (#2250) * Marking SimpleDb calls as idempotent (#2251) * Mark mostly readonly calls as idempotent (#2252) * Fix test failure (#2256) * Make handleUserChange idempotent (#2257) * Temporarily disable CountingQueryEngine tests (#2258) * Improve test hack (#2259) * Improve test hack * Comment in test hack * Make getNewDocumentChanges() idempotent (#2255) * Add onCommitted listeners for transactions (#2265) * Fix build * Fix Lint * Make applyRemoteEvent idempotent (#2263) * Make notifyLocalViewChanges idempotent (#2268) * Make releaseQuery idempotent (#2266) * Mark acknowledgeBatch and rejectBatch idempotent (#2269) * idempotent `allocateQuery` and `notifyLocalViewChanges` (#2264) * Mark collectGarbage idempotent (#2267) * Idempotency: Address TODOs, add Changelog (#2270)
This should simply some of the idempotent development by allowing any PersistenceTransaction consumer to register onCommitted listeners.