Add events to createSubDirectory and deleteSubDirectory #8842
Conversation
github-actions
bot
added
the
area: dds
| @@ -1519,6 +1519,16 @@ class SubDirectory extends TypedEventEmitter<IDirectoryEvents> implements IDirec | |||
| this.serializer, | |||
| posix.join(this.absolutePath, subdirName)), | |||
| ); | |||
|
|
|||
| const event: IDirectoryValueChanged = { | |||
There was a problem hiding this comment.
How listener can figure out if sub-directory was created, or a key was set with same name?
For create & delete events, I'd suggest defining and using separate interface
There was a problem hiding this comment.
And separate event names, of course :)
There was a problem hiding this comment.
That said, with current structure (of having valueChanged & containedValueChanged events) it would be hard as all events need to be duplicated.
There was a problem hiding this comment.
You're right - I see the problem with using the same event when keys and subdirectories are handled differently by everything else.
| path: this.absolutePath, | ||
| previousValue, | ||
| }; | ||
| this.directory.emit("valueChanged", event, local, this.directory); |
There was a problem hiding this comment.
This is continuation of the same thought: Listener can get a value of the key (let's say string) or some complex non-jsonable object (subdirectory) as a value for previousValue.
It might be too late, but I'd personally prefer to have valueChanged / subDirectoryCreated / subDirectoryDeleted events only (no duplication in the form of "containedValueChanged") and either
- have different objects to register for non-recursive and recursive notifications
- Add boolean to specify if registration is for recursive or not changes (this has some limitation - only last registration for given listener would stay active, in case caller registered same listener for recursive & non-recursive events).
There was a problem hiding this comment.
Agreed that subDirectoryCreated/subDirectoryDeleted would be better here. Otherwise all customers must put a conditional in their handler to figure out what they're working with, so better served by a more-specific event name. This will also avoid the breaking change -- as-is this will break customers who don't expect subdirectories to trigger these existing events.
There was a problem hiding this comment.
I see your point that valueChanged may not be suited for create/deleteSubDirectory. Previously I had conceptualized subdirectories as a specialized form of a key, but looking at the implementation more I can tell they are handled entirely differently.
I agree with the sentiment that valueChanged and containedValueChanged feel redundant and awkward. I think containedValueChanged is unique in that you can subscribe to events of a specific directory and not receive the events from the root directory every time, but something about the duplicated event feels off. We don't expect to use containedValueChanged for our needs.
| path: this.absolutePath, | ||
| previousValue, | ||
| }; | ||
| this.directory.emit("valueChanged", event, local, this.directory); |
There was a problem hiding this comment.
Agreed that subDirectoryCreated/subDirectoryDeleted would be better here. Otherwise all customers must put a conditional in their handler to figure out what they're working with, so better served by a more-specific event name. This will also avoid the breaking change -- as-is this will break customers who don't expect subdirectories to trigger these existing events.
| // This should make the subdirectory structure unreachable so it can be GC'd and won't appear in snapshots | ||
| // Might want to consider cleaning out the structure more exhaustively though? | ||
| return this._subdirectories.delete(subdirName); | ||
| if (this.hasSubDirectory(subdirName)) { |
There was a problem hiding this comment.
These events will suffer from the same core issue as #6069, that needProcessSubDirectoryOperations (and a couple other locations) has an insufficient design currently (treats resolution algorithm for directory operations as "last-write-wins" when they in fact need something more sophisticated). As a result neither the eventual consistency nor the events can be trusted until #6069 is resolved.
Because that resolution is more complex than "last-write-wins", we need a different event design. "valueChanged" and "containedValueChanged" (and their suppression while an op is outstanding) are sufficient for the LWW used on key values, but not good enough for subdirectories where ops prior to the ack of the "last write" still have an impact on the final outcome. See #8823 for an example where it's less clear what the eventing pattern should be, and some thoughts on design requirements.
|
Thanks for the PR. Closed with #9061 |
It's important that createSubDirectory and deleteSubDirectory emit events since they alter the state of the directory. I've implemented that through the existing valueChanged and containedValueChanged events, which I like because subdirectories can be treated like key:value pairs just like all the other directory key:values.