-
Notifications
You must be signed in to change notification settings - Fork 576
feat(tree): Allow event deferral during transactions #27238
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
68ff0da
673f492
4a3dbcd
8ebf653
c013633
e5e043e
5bdb9a2
269017d
78b29c1
e770ed6
fc7c10c
de65dd0
768fe14
70014a8
b282945
59fd862
7482cf2
4f9bc38
a4aac1d
b1e0ecf
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 |
|---|---|---|
|
|
@@ -307,6 +307,24 @@ type KernelEvents = Pick<AnchorEvents, (typeof kernelEvents)[number]>; | |
| */ | ||
| let bufferTreeEvents: boolean = false; | ||
|
|
||
| /** | ||
| * Options for {@link withBufferedTreeEvents}. | ||
| */ | ||
| export interface WithBufferedTreeEventsOptions<TResult> { | ||
| /** | ||
| * Predicate invoked with the callback's return value after it completes. | ||
| * If it returns `true`, all events buffered during this call are discarded | ||
| * instead of being flushed. | ||
| * @remarks | ||
| * Only honored at the outermost call. When `withBufferedTreeEvents` is invoked | ||
| * while another call is already buffering, this option has no effect — the | ||
| * discard decision is owned by the outer call. | ||
| * | ||
| * If the callback throws, the predicate is not invoked and buffered events are flushed. | ||
| */ | ||
| readonly shouldDiscard?: (result: TResult) => boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Call the provided callback with {@link TreeNode}s' events paused until after the callback's completion. | ||
| * | ||
|
|
@@ -316,27 +334,36 @@ let bufferTreeEvents: boolean = false; | |
| * @remarks | ||
| * Note: this should be used with caution. User application behaviors are implicitly coupled to event timing. | ||
| * Disrupting this timing can lead to unexpected behavior. | ||
| * @param callback - Function to invoke while events are buffered. | ||
| * @param options - Optional configuration. See {@link WithBufferedTreeEventsOptions}. | ||
| */ | ||
| export function withBufferedTreeEvents(callback: () => void): void { | ||
| export function withBufferedTreeEvents<TResult>( | ||
|
Contributor
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 think it might be cleaner to have the primary callback passed to The downside is that then you couldn't return the return value of the transaction directly, so you might have to add a few extra lines at the call site to capture the return value within the callback and have it outside. But the call site is already split up anyway to accommodate the current scheme: It's not like it's a super clean one liner, so I don't think it'd make it too much messier. And then this whole interface would get much simpler and self-describing. What do you think?
Contributor
Author
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. Let me play around with it and see how it works out. I think I like it. |
||
| callback: () => TResult, | ||
| options?: WithBufferedTreeEventsOptions<TResult>, | ||
| ): TResult { | ||
| if (bufferTreeEvents) { | ||
| // Already buffering - just run the callback | ||
| callback(); | ||
| } else { | ||
| bufferTreeEvents = true; | ||
| try { | ||
| callback(); | ||
| } finally { | ||
| bufferTreeEvents = false; | ||
| flushEventsEmitter.emit("flush"); | ||
| } | ||
| // Already buffering - just run the callback. The outermost call owns the flush/discard decision. | ||
| return callback(); | ||
|
Josmithr marked this conversation as resolved.
Outdated
|
||
| } | ||
| bufferTreeEvents = true; | ||
| let discard = false; | ||
| try { | ||
| const result = callback(); | ||
| discard = options?.shouldDiscard?.(result) === true; | ||
| return result; | ||
| } finally { | ||
| bufferTreeEvents = false; | ||
| flushEventsEmitter.emit(discard ? "discard" : "flush"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Event emitter to notify subscribers when tree events buffered due to {@link withBufferedTreeEvents} should be flushed. | ||
| * Event emitter to notify subscribers when tree events buffered due to {@link withBufferedTreeEvents} should be flushed | ||
| * or discarded. | ||
| */ | ||
| const flushEventsEmitter = createEmitter<{ | ||
| flush: () => void; | ||
| discard: () => void; | ||
| }>(); | ||
|
|
||
| /** | ||
|
|
@@ -351,6 +378,14 @@ class KernelEventBuffer implements Listenable<KernelEvents> { | |
| */ | ||
| readonly #disposeOnFlushListener = flushEventsEmitter.on("flush", this.flush.bind(this)); | ||
|
|
||
| /** | ||
| * Listen to {@link flushEventsEmitter} to know when to discard buffered events. | ||
| */ | ||
| readonly #disposeOnDiscardListener = flushEventsEmitter.on( | ||
| "discard", | ||
| this.discard.bind(this), | ||
| ); | ||
|
|
||
| readonly #events = createEmitter<KernelEvents>(); | ||
|
|
||
| #eventSource: Listenable<KernelEvents> & HasListeners<KernelEvents>; | ||
|
|
@@ -526,6 +561,11 @@ class KernelEventBuffer implements Listenable<KernelEvents> { | |
| public flush(): void { | ||
| this.#assertNotDisposed(); | ||
|
|
||
| // TODO: The buffer tracks *which* fields changed during the window but not the net delta, | ||
| // so a sequence of edits that nets to no change within a committed transaction (e.g. an | ||
| // insert followed by a remove of the same item) still emits one event per affected field. | ||
| // Suppressing those would require delta composition support in the eventing stack; see the | ||
| // invalidation comment in #handleChildrenChangedAfterBatch for the related limitation. | ||
| if (this.#childrenChangedBuffer.size > 0) { | ||
| this.#events.emit("childrenChangedAfterBatch", { | ||
| changedFields: this.#childrenChangedBuffer, | ||
|
|
@@ -542,6 +582,22 @@ class KernelEventBuffer implements Listenable<KernelEvents> { | |
| } | ||
| } | ||
|
Josmithr marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * Discards any events buffered due to {@link withBufferedTreeEvents} without emitting them. | ||
| * | ||
| * @remarks | ||
| * Used by transaction code paths that know the tree is in the same state it started in | ||
| * (e.g. a rolled-back synchronous transaction) so the buffered events represent net-zero | ||
| * changes that should not be observed by listeners. | ||
| */ | ||
| public discard(): void { | ||
| this.#assertNotDisposed(); | ||
| this.#childrenChangedBuffer.clear(); | ||
| this.#fieldMarksBuffer.clear(); | ||
| this.#invalidatedFieldMarkKeys.clear(); | ||
| this.#subTreeChangedBuffer = false; | ||
| } | ||
|
|
||
| #assertNotDisposed(): void { | ||
| assert(!this.#disposed, 0xc51 /* Event handler disposed. */); | ||
| } | ||
|
|
@@ -557,6 +613,7 @@ class KernelEventBuffer implements Listenable<KernelEvents> { | |
| ); | ||
|
|
||
| this.#disposeOnFlushListener(); | ||
| this.#disposeOnDiscardListener(); | ||
| for (const off of this.#disposeSourceListeners.values()) { | ||
| off(); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.