Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,16 @@ export class TreeNodeKernel {
#hydrationState: HydrationState;

/**
* Events registered before hydration.
* Handler for events listeners registered with the kernel.
*
* @remarks
* Since these are usually not used, they are allocated lazily as an optimization.
* The laziness also avoids extra forwarding overhead for events from this kernel's anchor node and also avoids registering for events that are unneeded.
* This means optimizations like skipping processing data in subtrees where no subtreeChanged events are subscribed to would be able to work,
* since the kernel does not unconditionally subscribe to those events (like a design which simply forwards all events would).
* Supports event buffering via {@link withBufferedEvents}.
*
* Allocated lazily on first access to {@link TreeNodeKernel.events}.
* We expect the majority of nodes to never have event listeners registered, so
* deferring construction avoids per-kernel allocations.
*/
readonly #eventBuffer: KernelEventBuffer;
#eventBuffer: KernelEventBuffer | undefined;
Comment thread
Josmithr marked this conversation as resolved.

/**
* Create a TreeNodeKernel which can be looked up with {@link getKernel}.
Expand Down Expand Up @@ -157,12 +159,9 @@ export class TreeNodeKernel {
this.#hydrationState = {
innerNode,
};

this.#eventBuffer = new KernelEventBuffer(innerNode.events);
} else {
// Hydrated case
this.#hydrationState = this.createHydratedState(innerNode);
this.#eventBuffer = new KernelEventBuffer(innerNode.anchorNode.events);
}
}

Expand Down Expand Up @@ -190,8 +189,10 @@ export class TreeNodeKernel {

this.#hydrationState = this.createHydratedState(inner);

// Lazily migrate existing event listeners to the anchor node
this.#eventBuffer.migrateEventSource(inner.anchorNode.events);
// Lazily migrate existing event listeners to the anchor node.
// If no one ever subscribed to this kernel's events, the buffer was never allocated
// and there is nothing to migrate.
this.#eventBuffer?.migrateEventSource(inner.anchorNode.events);
}

private createHydratedState(innerNode: HydratedFlexTreeNode): HydratedState {
Expand Down Expand Up @@ -233,6 +234,14 @@ export class TreeNodeKernel {
}

public get events(): Listenable<KernelEvents> {
assert(!this.disposed, "Cannot register events on a disposed node");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this also use this.assertNotDisposed()?

// Allocate the buffer on first access. See {@link TreeNodeKernel.#eventBuffer} for rationale.
if (this.#eventBuffer === undefined) {
const eventSource = isHydrated(this.#hydrationState)
? this.#hydrationState.innerNode.anchorNode.events
: this.#hydrationState.innerNode.events;
this.#eventBuffer = new KernelEventBuffer(eventSource);
}
return this.#eventBuffer;
Comment thread
Josmithr marked this conversation as resolved.
}

Expand All @@ -244,7 +253,7 @@ export class TreeNodeKernel {
off();
}
}
this.#eventBuffer.dispose();
this.#eventBuffer?.dispose();
// TODO: go to the context and remove myself from withAnchors
}

Expand Down Expand Up @@ -540,6 +549,8 @@ class KernelEventBuffer implements Listenable<KernelEvents> {
}

public on(eventName: keyof KernelEvents, listener: KernelEvents[typeof eventName]): Off {
this.#assertNotDisposed();

// Lazily bind event listeners to the source.
// If we do not have any existing listeners for this event, then we need to bind to the source.
if (!this.#events.hasListeners(eventName)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,42 @@ describe("simple-tree proxies", () => {
assert.equal(anchors.find(path), undefined);
assert(anchors.isEmpty());
});

it("can hydrate a node with existing event listeners", () => {
// Listeners registered before hydration must continue to fire after the
// kernel migrates its event source from the unhydrated inner node to the
// hydrated anchor node.
const node = new ChildSchema({ content: 1 });

const log: string[] = [];
TreeBeta.on(node, "nodeChanged", ({ changedProperties }) => {
log.push(`nodeChanged: ${JSON.stringify([...changedProperties.keys()].sort())}`);
});

// Mutating before hydration: listener fires from the unhydrated event source.
node.content = 2;
assert.deepEqual(log, ['nodeChanged: ["content"]']);

hydrate(ChildSchema, node);

// Mutating after hydration: listener must continue firing, now from the anchor-node source.
node.content = 3;
assert.deepEqual(log, ['nodeChanged: ["content"]', 'nodeChanged: ["content"]']);
});

it("registering event listeners on a disposed kernel throws", () => {
// Once a kernel has been disposed (e.g. via afterDestroy on its anchor node),
// accessing its events to subscribe must fail loudly rather than silently
// allocating a fresh buffer on a defunct kernel.
const node = new ChildSchema({ content: 1 });
hydrate(ChildSchema, node);
getKernel(node).dispose();

assert.throws(
() => TreeBeta.on(node, "nodeChanged", () => {}),
/Cannot register events on a disposed node/,
);
});
});

describe("withBufferedTreeEvents", () => {
Expand Down Expand Up @@ -151,6 +187,22 @@ describe("withBufferedTreeEvents", () => {
assert.equal(eventCounter, 1); // Only a single event should have been raised.
});

it("subscribe then unsubscribe inside a scope emits no events", () => {
// A buffer that's been allocated lazily, then drained of listeners before the
// scope closes, must pop cleanly without emitting anything.
const obj = hydrate(MyObject, new MyObject({ foo: "init", bar: true }));
const log: string[] = [];

withBufferedTreeEvents(() => {
const off = TreeBeta.on(obj, "nodeChanged", ({ changedProperties }) => {
log.push(`nodeChanged: ${JSON.stringify([...changedProperties.keys()].sort())}`);
});
obj.foo = "outer";
off();
});
assert.deepEqual(log, []);
});

describe("nested calls", () => {
// Helper: build a fresh logged subject for each test.
function makeSubject(): {
Expand Down
Loading