Add optional dispose flag for forks after transaction

[daesunp]

Description

This PR adds a flag to not dispose of the fork after the transaction is commited/aborted.

[noencke]

How about "SharedTreeOptionsInternal"?

[daesunp]

Updated to suggested name

[noencke]

Suggested change

	private readonly disposeForksAfterTransaction?: boolean,
	private readonly disposeForksAfterTransaction = false,

If you default on initialization, then it will be a boolean rather than a boolean | undefined, and any code in the class that reads it doesn't need to worry about handling undefined.

[daesunp]

updated to default on initialization

[noencke]

Since you're not doing this disposal, you shouldn't need any of the fork tracking code above either. The only reason we subscribe to the fork event is to do this disposal. If disposeForksAfterTransaction is false, then probably just have forks be undefined or something.

[daesunp]

Updated to only track forks when dispose flag is true

[noencke]

There is still some code running that doesn't need to. How about this: move all of the code related to the forking stuff into a nice, encapulated helper function. Something like:

// Keeps track of all new forks created until the returned function is invoked, which will dispose all of those forks
// The returned function may only be called once
function trackForksForDisposal(checkout: TreeCheckout): () => void {
    const forks = new Set<TreeCheckout>();
    let onForkUnSubscribe: () => void | undefined;
    const onDisposeUnSubscribes: (() => void)[] = [];
    onForkUnSubscribe = onForkTransitive(checkout, (fork) => {
        forks.add(fork);
        onDisposeUnSubscribes.push(fork.events.on("dispose", () => forks.delete(fork)));
    });
    let disposed = false;
    return () => {
        assert(!disposed, "Forks may only be disposed once");
        forks.forEach((fork) => fork.dispose());
        onDisposeUnSubscribes.forEach((unsubscribe) => unsubscribe());
        onForkUnSubscribe();
        disposed = true;
    }
}

Then you can just do:

const disposeForks = this.disposeForksAfterTransaction ? collectForksForDisposal(this) : undefined;
// ...
disposeForks?.();

[daesunp]

For some reason, it does not seem to populate the forks properly when combining everything into one function. I have created 2 helper functions to simplify it a bit though

[noencke]

Huh. Why? It should be a pretty straightforward code move 🤔

The way it is now is more complicated and verbose than either the original state or the above suggestion, IMO.

[daesunp]

My mistake! Turns out I was calling trackForksForDisposal in the wrong place (within the return of the constructor). It seems to be working properly :)

[noencke]

There is still some code running that doesn't need to. How about this: move all of the code related to the forking stuff into a nice, encapulated helper function. Something like:

// Keeps track of all new forks created until the returned function is invoked, which will dispose all of those forks
// The returned function may only be called once
function trackForksForDisposal(checkout: TreeCheckout): () => void {
    const forks = new Set<TreeCheckout>();
    let onForkUnSubscribe: () => void | undefined;
    const onDisposeUnSubscribes: (() => void)[] = [];
    onForkUnSubscribe = onForkTransitive(checkout, (fork) => {
        forks.add(fork);
        onDisposeUnSubscribes.push(fork.events.on("dispose", () => forks.delete(fork)));
    });
    let disposed = false;
    return () => {
        assert(!disposed, "Forks may only be disposed once");
        forks.forEach((fork) => fork.dispose());
        onDisposeUnSubscribes.forEach((unsubscribe) => unsubscribe());
        onForkUnSubscribe();
        disposed = true;
    }
}

Then you can just do:

const disposeForks = this.disposeForksAfterTransaction ? collectForksForDisposal(this) : undefined;
// ...
disposeForks?.();

[noencke]

Is this better?

Suggested change

	super(options);
	super({ ...options, jsonValidator: typeboxValidator });

You want this to use typeboxValidator no matter what, correct? Otherwise it's broken? Or, is there a legitimate use case for a subclass of this which allows a different validator?

And if so, you'd still want to supply typeboxValidator if the subclass didn't supply anything. So that'd at least be:

super({ ...options, jsonValidator: options.jsonValidator ?? typeboxValidator })

In either case, you can change the default value for the options parameter to be {}, and you should follow the same pattern in the other constructor of your new subclass (because it could be subclassed).

[daesunp]

Updated with suggestion

[noencke]

Nice!

[noencke]

nit: one of the benefits of factoring code out into helper functions is that you can put doc-comments on them. So let's take advantage of that and make this a full-on doc comment.

[daesunp]

added doc comment to helper function