-
Notifications
You must be signed in to change notification settings - Fork 945
Add onCommitted listeners for transactions #2265
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
Changes from 1 commit
62a306a
351bebb
e78cfd5
6854745
8714404
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,14 +29,25 @@ import { RemoteDocumentCache } from './remote_document_cache'; | |
import { ClientId } from './shared_client_state'; | ||
|
||
/** | ||
* Opaque interface representing a persistence transaction. | ||
* A base class representing a persistence transaction, encapsulating both the | ||
* transaction's sequence numbers as well as a list of onCommitted listeners. | ||
* | ||
* When you call Persistence.runTransaction(), it will create a transaction and | ||
* pass it to your callback. You then pass it to any method that operates | ||
* on persistence. | ||
*/ | ||
export abstract class PersistenceTransaction { | ||
private readonly onCommittedListeners: Array<() => {}> = []; | ||
|
||
abstract readonly currentSequenceNumber: ListenSequenceNumber; | ||
|
||
addOnCommittedListener(listener: () => {}): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we require that the committed listeners return Or would that interfere with using short arrow functions in the callback? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized this when I started to modify my "applyRemoteEvent" PR. It is much better if we return void. |
||
this.onCommittedListeners.push(listener); | ||
} | ||
|
||
raiseOnCommittedEvent(): void { | ||
this.onCommittedListeners.forEach(listener => listener()); | ||
} | ||
} | ||
|
||
/** The different modes supported by `IndexedDbPersistence.runTransaction()`. */ | ||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to invoke the committed handler as the last step.