Update internal SharedTree types to use the .events pattern#23038
Conversation
github-actions
Bot
added
area: dds
| type TreeFieldStoredSchema, | ||
| ValueSchema, | ||
| TreeNodeStoredSchema, | ||
| type TreeStoredSchemaSubscription as TreeStoredSchemaSubscription, |
There was a problem hiding this comment.
This change is unrelated to the rest of this PR but I stumbled across it and it didn't seem worth checking in as its own PR.
| export interface IForestSubscription extends Listenable<ForestEvents> { | ||
| export interface IForestSubscription { | ||
| /** | ||
| * Events for this forest. |
There was a problem hiding this comment.
For our internal types, I don't actually think it's practically necessary to document the events property at either the interface level or the implementation level since it's clear what is going on (and it's the events themselves, not the event emitter, that needs documentation). Nonetheless I added a brief doc comment in places where the surrounding properties also had consistent doc comments.
| forkable: T, | ||
| // Branches are invariant over TChange | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function onForkTransitive<T extends SharedTreeBranch<ChangeFamilyEditor, any>>( |
There was a problem hiding this comment.
This function was typed as it was to avoid having to use a generic any, but I decided it's just simpler to add a lint exception and make it hardcoded to the branch type. That also helped with some typing elsewhere in this class where onForkTransitive was being used that were acting up after switching to .events.
| protected readonly _events = createEmitter<SchemaEvents>(); | ||
| public readonly events: Listenable<SchemaEvents> = this._events; |
There was a problem hiding this comment.
What's the motivation for having _events?
There was a problem hiding this comment.
Is that to keep the ability to fire events encapsulated?
There was a problem hiding this comment.
Yes, exactly. Usually it's #events, but in this case it's protected so we use _events.
There was a problem hiding this comment.
In the future I'd like to explore if there is some clever way to have it work without having to define multiple fields, but I'm not sure if that's possible.
There was a problem hiding this comment.
Yeah, I was wondering the same thing.
Having usages of the class be indirected through an interface would take care of it.
Otherwise, I would find this pattern less ugly if we had one public member for listerning only and one private member for firing only.
You'd then initialize them in the ctor by doing {this.events, this.eventEmitter} = createEmitter<SchemaEvents>(); or something like that.
There was a problem hiding this comment.
That's interesting! That could work. Although, I don't think it's possible to do the destructuring in one line like that - you'd have to use a local and at least three lines. But the splitting of the two does make sense.
Another option would be to do public readonly events = createEmitter() and have it already be the "listen-only" interface, but there's a static helper emit(this.events, key, ...) which does a cast inside to get the emitting version. That would be the least amount of boilerplate, but is kind of unconventional and weird (even if we ensured the cast was safe by e.g. using a listen-only class instead of merely a listen-only interface).
There was a problem hiding this comment.
Nit: eventEmitter might be a better property name for these to help clarify the difference
| function subscribeToLocalBranch<TChange>( | ||
| manager: EditManager<ChangeFamilyEditor, TChange, ChangeFamily<ChangeFamilyEditor, TChange>>, | ||
| ): void { | ||
| manager.localBranch.events.on("afterChange", (branchChange) => { | ||
| // Reading the change property causes lazy computation to occur, and is important to accurately emulate SharedTree behavior | ||
| const _change = branchChange.change; | ||
| }); |
There was a problem hiding this comment.
Much improved, thank you!
msfluid-bot
left a comment
msfluid-bot
left a comment
There was a problem hiding this comment.
Code Coverage Summary
↓ packages.dds.tree.src.core.schema-stored:
Line Coverage Change: No change Branch Coverage Change: -0.14%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 93.75% | 93.61% | ↓ -0.14% |
| Line Coverage | 99.70% | 99.70% | → No change |
↓ packages.dds.tree.src.feature-libraries.object-forest:
Line Coverage Change: -0.02% Branch Coverage Change: -0.06%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 95.40% | 95.34% | ↓ -0.06% |
| Line Coverage | 97.66% | 97.64% | ↓ -0.02% |
↓ packages.dds.tree.src.feature-libraries.chunked-forest:
Line Coverage Change: -0.01% Branch Coverage Change: -0.03%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 86.98% | 86.95% | ↓ -0.03% |
| Line Coverage | 95.62% | 95.61% | ↓ -0.01% |
↓ packages.dds.tree.src.core.tree:
Line Coverage Change: -0.02% Branch Coverage Change: -0.02%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 96.36% | 96.34% | ↓ -0.02% |
| Line Coverage | 88.11% | 88.09% | ↓ -0.02% |
↑ packages.dds.tree.src.core.forest:
Line Coverage Change: 0.01% Branch Coverage Change: No change
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 100.00% | 100.00% | → No change |
| Line Coverage | 99.48% | 99.49% | ↑ 0.01% |
↑ packages.dds.tree.src.simple-tree:
Line Coverage Change: No change Branch Coverage Change: 0.01%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 94.06% | 94.07% | ↑ 0.01% |
| Line Coverage | 97.21% | 97.21% | → No change |
↑ packages.dds.tree.src.shared-tree:
Line Coverage Change: No change Branch Coverage Change: 0.03%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 90.85% | 90.88% | ↑ 0.03% |
| Line Coverage | 97.28% | 97.28% | → No change |
↑ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: No change Branch Coverage Change: 0.03%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 88.73% | 88.76% | ↑ 0.03% |
| Line Coverage | 82.19% | 82.19% | → No change |
↑ packages.dds.tree.src.feature-libraries.flex-tree:
Line Coverage Change: 0.17% Branch Coverage Change: No change
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 93.42% | 93.42% | → No change |
| Line Coverage | 90.06% | 90.23% | ↑ 0.17% |
↑ packages.dds.tree.src.events:
Line Coverage Change: 1.10% Branch Coverage Change: -0.34%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 92.00% | 91.66% | ↓ -0.34% |
| Line Coverage | 70.55% | 71.65% | ↑ 1.10% |
Baseline commit: bb64a8e
Baseline build: 305482
Happy Coding!!
Code coverage comparison check passed!!
⯅ @fluid-example/bundle-size-tests: +3.05 KB
Baseline commit: bb64a8e |
| * private readonly _events = createEmitter<MyEvents>(); | ||
| * public readonly events: Listenable<MyEvents> = this._events; |
There was a problem hiding this comment.
| * private readonly _events = createEmitter<MyEvents>(); | |
| * public readonly events: Listenable<MyEvents> = this._events; | |
| * private readonly _events = createEmitter<MyEvents>(); | |
| * public readonly events: Listenable<MyEvents> = this._events; |
There was a problem hiding this comment.
Ugh - that's weird. Thanks. I'll fix it in the next event PR.
4 participants
Description
This changes all internal SharedTree objects to have an
events: Listenableproperty rather than implementing Listenable directly. Using the.eventspattern is preferable over the alternatives because it does not employ inheritance (like extendingEventEmitter) and does not require any method implementation boilerplate (like implementingListenable). It also means that any changes toEventEmitterorListenablewill not require changes to the object emitting the events - this is the practical motivation for this change, as theListenableinterface will soon be updated to have a new method. Without this preparatory change, all the implementations ofListenablewould need to be updated at that time.