diff --git a/model-client/src/jsMain/kotlin/org/modelix/model/client2/ChangeJS.kt b/model-client/src/jsMain/kotlin/org/modelix/model/client2/ChangeJS.kt index a3846cbff4..0efe45b8d8 100644 --- a/model-client/src/jsMain/kotlin/org/modelix/model/client2/ChangeJS.kt +++ b/model-client/src/jsMain/kotlin/org/modelix/model/client2/ChangeJS.kt @@ -61,3 +61,20 @@ data class ContainmentChanged(override val node: INodeJS) : ChangeJS */ @JsExport data class ConceptChanged(override val node: INodeJS) : ChangeJS + +/** + * Represents an added [node]. + * Every node of an added subtree is reported, not only the root of the subtree. + * Only reported to handlers added with [MutableModelTreeJs.addListenerIncludingAddedAndRemovedNodes]. + */ +@JsExport +data class NodeAdded(override val node: INodeJS) : ChangeJS + +/** + * Represents a removed [node]. + * Every node of a removed subtree is reported, not only the root of the subtree. + * Only reported to handlers added with [MutableModelTreeJs.addListenerIncludingAddedAndRemovedNodes]. + * Only the reference of [node] can be read, because it is no longer part of the model. + */ +@JsExport +data class NodeRemoved(override val node: INodeJS) : ChangeJS diff --git a/model-client/src/jsMain/kotlin/org/modelix/model/client2/ClientJS.kt b/model-client/src/jsMain/kotlin/org/modelix/model/client2/ClientJS.kt index 82ae920a30..3e0e9f6580 100644 --- a/model-client/src/jsMain/kotlin/org/modelix/model/client2/ClientJS.kt +++ b/model-client/src/jsMain/kotlin/org/modelix/model/client2/ClientJS.kt @@ -576,8 +576,22 @@ interface MutableModelTreeJs { */ fun addListener(handler: ChangeHandler) + /** + * Like [addListener], but the change handler is also called with [NodeAdded] and [NodeRemoved] + * for every node of an added or removed subtree. + * The order of these changes within one change of the branch, + * also relative to the other changes, is not specified. + * The handler is called more often than with [addListener], + * and the changes of the branch are computed once more for each such handler, + * so use it only when added or removed nodes have to be known. + * Once a handler is added with this method, it gets these changes until it is removed with [removeListener], + * even if it is also added with [addListener]. + */ + fun addListenerIncludingAddedAndRemovedNodes(handler: ChangeHandler) + /** * Remove a change handler from the branch. + * It removes handlers added with [addListener] and with [addListenerIncludingAddedAndRemovedNodes]. */ fun removeListener(handler: ChangeHandler) } diff --git a/model-client/src/jsMain/kotlin/org/modelix/model/client2/MutableModelTreeJsImpl.kt b/model-client/src/jsMain/kotlin/org/modelix/model/client2/MutableModelTreeJsImpl.kt index 8f6cfa4cb9..0e37c67212 100644 --- a/model-client/src/jsMain/kotlin/org/modelix/model/client2/MutableModelTreeJsImpl.kt +++ b/model-client/src/jsMain/kotlin/org/modelix/model/client2/MutableModelTreeJsImpl.kt @@ -29,6 +29,11 @@ internal class MutableModelTreeJsImpl( private val changeHandlers = mutableSetOf() + // The listeners that report [NodeAdded] and [NodeRemoved] to the handlers added with + // [addListenerIncludingAddedAndRemovedNodes], one per tree. + // They are only created and registered for these handlers, so that other handlers cost nothing extra. + private var addedAndRemovedNodesListeners: MutableMap>>? = null + private val model = CompositeModel(trees.map { it.asModel() }).withAutoTransactions() private val changeListeners = trees.map { tree -> ChangeListener(tree.withAutoTransactions()) { change -> @@ -51,8 +56,18 @@ internal class MutableModelTreeJsImpl( override fun addListener(handler: ChangeHandler) { changeHandlers.add(handler) } + override fun addListenerIncludingAddedAndRemovedNodes(handler: ChangeHandler) { + changeHandlers.add(handler) + val listeners = addedAndRemovedNodesListeners ?: mutableMapOf>>() + .also { addedAndRemovedNodesListeners = it } + if (handler in listeners) return + listeners[handler] = trees.map { tree -> + tree to AddedAndRemovedNodesListener(tree.withAutoTransactions(), handler).also { tree.addListener(it) } + } + } override fun removeListener(handler: ChangeHandler) { changeHandlers.remove(handler) + addedAndRemovedNodesListeners?.remove(handler)?.forEach { (tree, listener) -> tree.removeListener(listener) } } private fun IWritableNode.toJS() = toNodeJs(this.withAutoTransactions().asLegacyNode()) @@ -79,3 +94,25 @@ internal class ChangeListener(private val tree: IMutableModelTree, private val c } } } + +/** + * Reports only [NodeAdded] and [NodeRemoved] to [changeCallback]. + * The other changes are reported by [ChangeListener]. + */ +internal class AddedAndRemovedNodesListener(private val tree: IMutableModelTree, private val changeCallback: (ChangeJS) -> Unit) : + IGenericMutableModelTree.Listener { + + private fun nodeIdToInode(nodeId: INodeReference): INodeJS { + return toNodeJs(NodeInMutableModel(tree, nodeId).asLegacyNode()) + } + + override fun treeChanged(oldTree: IGenericModelTree, newTree: IGenericModelTree) { + newTree.getChanges(oldTree, false).iterateBlocking(newTree) { + when (it) { + is NodeAddedEvent -> changeCallback(NodeAdded(nodeIdToInode(it.nodeId))) + is NodeRemovedEvent -> changeCallback(NodeRemoved(nodeIdToInode(it.nodeId))) + else -> {} + } + } + } +} diff --git a/model-client/src/jsTest/kotlin/org/modelix/model/client2/MutableModelTreeJsTest.kt b/model-client/src/jsTest/kotlin/org/modelix/model/client2/MutableModelTreeJsTest.kt index 609e3350c1..019c622399 100644 --- a/model-client/src/jsTest/kotlin/org/modelix/model/client2/MutableModelTreeJsTest.kt +++ b/model-client/src/jsTest/kotlin/org/modelix/model/client2/MutableModelTreeJsTest.kt @@ -1,9 +1,21 @@ package org.modelix.model.client2 import GeneratedConcept +import org.modelix.datastructures.model.IGenericModelTree +import org.modelix.model.api.INodeReference +import org.modelix.model.client.IdGenerator +import org.modelix.model.data.ModelData +import org.modelix.model.lazy.createObjectStoreCache +import org.modelix.model.mutable.IGenericMutableModelTree +import org.modelix.model.mutable.IMutableModelTree +import org.modelix.model.mutable.ModelixIdGenerator +import org.modelix.model.mutable.asMutableThreadSafe +import org.modelix.model.mutable.load +import org.modelix.model.persistent.MapBasedStore import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNull +import kotlin.test.assertTrue class MutableModelTreeJsTest { @@ -201,4 +213,202 @@ class MutableModelTreeJsTest { // Assert assertEquals(1, childrenChanged) } + + @Test + fun changeHandlerIncludingAddedAndRemovedNodesGetsEveryNodeOfARemovedSubtree() { + // Arrange + val data = """ + { + "root": { + "children": [ + { + "id": "subtreeRoot", + "children": [ + { + "id": "descendant" + } + ] + } + ] + } + } + """.trimIndent() + val branch = loadModelsFromJsonAsBranch(arrayOf(data)) + val subtreeRoot = branch.rootNode.getAllChildren()[0] + val descendant = subtreeRoot.getAllChildren()[0] + var childrenChanged = 0 + val removedNodeReferences = mutableListOf() + branch.addListenerIncludingAddedAndRemovedNodes { + when (it) { + is ChildrenChanged -> childrenChanged++ + // The reference of a removed node can still be read. + is NodeRemoved -> removedNodeReferences.add(it.node.getReference()) + else -> {} + } + } + var changeCount = 0 + branch.addListener { changeCount++ } + + // Act + branch.rootNode.removeChild(subtreeRoot) + + // Assert + assertEquals(1, childrenChanged) + assertEquals( + setOf(subtreeRoot.getReference(), descendant.getReference()), + removedNodeReferences.toSet(), + ) + assertEquals(2, removedNodeReferences.size) + // A handler added with `addListener` still gets only the change to the children. + assertEquals(1, changeCount) + } + + @Test + fun changeHandlerIncludingAddedAndRemovedNodesGetsAnAddedNode() { + // Arrange + val branch = loadModelsFromJsonAsBranch(arrayOf(emptyRoot)) + val addedNodeReferences = mutableListOf() + val changeHandler: ChangeHandler = { + if (it is NodeAdded) { + addedNodeReferences.add(it.node.getReference()) + } + } + branch.addListenerIncludingAddedAndRemovedNodes(changeHandler) + // Adding the handler again with `addListener` does not opt it out. + branch.addListener(changeHandler) + + // Act + val childNode = branch.rootNode.addNewChild("aRole", -1, GeneratedConcept("aConceptUid")) + + // Assert + assertEquals(listOf(childNode.getReference()), addedNodeReferences) + } + + @Test + fun changeHandlersAreCalledInTheOrderTheyWereAdded() { + // Arrange + val branch = loadModelsFromJsonAsBranch(arrayOf(emptyRoot)) + val calledHandlers = mutableListOf() + branch.addListenerIncludingAddedAndRemovedNodes { calledHandlers.add("includingAddedAndRemovedNodes") } + branch.addListener { calledHandlers.add("plain") } + + // Act + branch.rootNode.setPropertyValue("aProperty", "aValue") + + // Assert + assertEquals(listOf("includingAddedAndRemovedNodes", "plain"), calledHandlers) + } + + @Test + fun changeHandlerIncludingAddedAndRemovedNodesCanBeRemoved() { + val branch = loadModelsFromJsonAsBranch(arrayOf(rootWithChild)) + var changeCount = 0 + val changeListener: ChangeHandler = { _ -> changeCount++ } + branch.addListenerIncludingAddedAndRemovedNodes(changeListener) + branch.removeListener(changeListener) + + val aNode = branch.rootNode.getAllChildren()[0] + branch.rootNode.removeChild(aNode) + + assertEquals(0, changeCount) + } + + @Test + fun changeHandlerIncludingAddedAndRemovedNodesGetsEveryNodeOfAnAddedSubtree() { + // Arrange + val tree = ListenerTrackingTree(createEmptyTree()) + val branch = MutableModelTreeJsImpl(tree) + var childrenChanged = 0 + val addedNodeReferences = mutableListOf() + branch.addListenerIncludingAddedAndRemovedNodes { + when (it) { + is ChildrenChanged -> childrenChanged++ + is NodeAdded -> addedNodeReferences.add(it.node.getReference()) + else -> {} + } + } + val data = """ + { + "root": { + "children": [ + { + "id": "subtreeRoot", + "children": [ + { + "id": "descendant" + } + ] + } + ] + } + } + """.trimIndent() + + // Act + ModelData.fromJson(data).load(tree) + + // Assert + val subtreeRoot = branch.rootNode.getAllChildren()[0] + val descendant = subtreeRoot.getAllChildren()[0] + assertEquals( + setOf(subtreeRoot.getReference(), descendant.getReference()), + addedNodeReferences.toSet(), + ) + assertEquals(2, addedNodeReferences.size) + assertEquals(1, childrenChanged) + } + + @Test + fun addedAndRemovedNodesAreOnlyReportedWhileAHandlerIncludingThemExists() { + // Arrange + val tree = ListenerTrackingTree(createEmptyTree()) + val branch = MutableModelTreeJsImpl(tree) + val changes = mutableListOf() + branch.addListener { changes.add(it) } + val handlerIncludingAddedAndRemovedNodes: ChangeHandler = {} + + // Act + val childNode = branch.rootNode.addNewChild("aRole", -1, GeneratedConcept("aConceptUid")) + branch.rootNode.removeChild(childNode) + + // Assert + // Only the listener for the other changes is registered, so added and removed nodes are not even looked up. + assertEquals(1, tree.listeners.size) + assertEquals(2, changes.size) + assertTrue(changes.none { it is NodeAdded || it is NodeRemoved }) + + // Act + branch.addListenerIncludingAddedAndRemovedNodes(handlerIncludingAddedAndRemovedNodes) + + // Assert + assertEquals(2, tree.listeners.size) + + // Act + branch.removeListener(handlerIncludingAddedAndRemovedNodes) + + // Assert + assertEquals(1, tree.listeners.size) + } + + private fun createEmptyTree(): IMutableModelTree { + val tree = IGenericModelTree.builder() + .graph(createObjectStoreCache(MapBasedStore()).asObjectGraph()) + .withNodeReferenceIds() + .build() + return tree.asMutableThreadSafe(ModelixIdGenerator(IdGenerator.getInstance(1), tree.getId())) + } + + private class ListenerTrackingTree(private val tree: IMutableModelTree) : IMutableModelTree by tree { + val listeners = mutableListOf>() + + override fun addListener(listener: IGenericMutableModelTree.Listener) { + listeners.add(listener) + tree.addListener(listener) + } + + override fun removeListener(listener: IGenericMutableModelTree.Listener) { + listeners.remove(listener) + tree.removeListener(listener) + } + } } diff --git a/vue-model-api/src/ReactiveModelOptions.ts b/vue-model-api/src/ReactiveModelOptions.ts new file mode 100644 index 0000000000..37896d5457 --- /dev/null +++ b/vue-model-api/src/ReactiveModelOptions.ts @@ -0,0 +1,15 @@ +/** + * Options for the reactive nodes returned by `useModelsFromJson`, `useReplicatedModel` + * and `useReplicatedModels`. + */ +export interface ReactiveModelOptions { + /** + * Make `getReferenceTargetNode` read a reference to a removed node as unset. + * `getReferenceTargetRef` still returns the reference. + * Without it, a view that shows such a reference keeps showing the removed node and does not update. + * With it, every node of every added or removed subtree is looked up, + * for example, for a large merge from the server, + * so adding or removing many nodes at once costs more. Default: `false`. + */ + updateReferencesToRemovedNodes?: boolean; +} diff --git a/vue-model-api/src/index.ts b/vue-model-api/src/index.ts index 1078102bd1..b8de2df502 100644 --- a/vue-model-api/src/index.ts +++ b/vue-model-api/src/index.ts @@ -3,3 +3,4 @@ export { useModelClient } from "./useModelClient"; export { useReplicatedModels } from "./useReplicatedModels"; export { useReplicatedModel } from "./useReplicatedModel"; export { useReadonlyVersion } from "./useReadonlyVersion"; +export type { ReactiveModelOptions } from "./ReactiveModelOptions"; diff --git a/vue-model-api/src/internal/ReactiveINodeJS.test.ts b/vue-model-api/src/internal/ReactiveINodeJS.test.ts index d0e4361e95..6fc041f138 100644 --- a/vue-model-api/src/internal/ReactiveINodeJS.test.ts +++ b/vue-model-api/src/internal/ReactiveINodeJS.test.ts @@ -1,8 +1,19 @@ import { useModelsFromJson } from "../useModelsFromJson"; import { computed, isReactive, reactive } from "vue"; import { runGarbageCollection } from "./runGarbageCollection"; +import type { INodeJS } from "@modelix/ts-model-api"; import { toRoleJS } from "@modelix/ts-model-api"; import { ReadOnlyNodeJS } from "../ReadonlyNodeJS"; +import { org } from "@modelix/model-client"; +import { handleAddedOrRemovedNode } from "./handleChange"; +import { + RemovalTrackingCache, + RemovalTrackingReactiveINodeJS, + toReactiveINodeJS, +} from "./ReactiveINodeJS"; +import type { ReactiveModelOptions } from "../ReactiveModelOptions"; + +const { NodeAdded, NodeRemoved } = org.modelix.model.client2; const root = { root: { @@ -202,3 +213,305 @@ test("can setReferenceTargetNode to a readonly node", async () => { child0.getReferenceTargetNode(toRoleJS("aReference"))?.getReference(), ).toEqual(child2.getReference()); }); + +const modelWithReference = { + root: { + children: [ + { + role: "holders", + references: { aReference: "target" }, + }, + { + role: "holders", + properties: { name: "otherHolder" }, + }, + { + role: "containers", + children: [ + { + id: "target", + role: "items", + properties: { name: "target" }, + }, + ], + }, + ], + }, +}; + +const updateReferencesToRemovedNodes = { updateReferencesToRemovedNodes: true }; + +// The holder is read through its parent, the target only through the reference, +// so the target's parent "container" is never cached. +function useModelWithReference( + options: ReactiveModelOptions = updateReferencesToRemovedNodes, +) { + const rootNode = useModelsFromJson( + [JSON.stringify(modelWithReference)], + options, + ); + const [holder, otherHolder] = rootNode.getChildren(toRoleJS("holders")); + return { rootNode, holder, otherHolder }; +} + +test("reference to a deleted node reads as unset", () => { + const { holder } = useModelWithReference(); + const computedTargetName = computed(() => + holder + .getReferenceTargetNode(toRoleJS("aReference")) + ?.getPropertyValue(toRoleJS("name")), + ); + expect(computedTargetName.value).toBe("target"); + + holder.getReferenceTargetNode(toRoleJS("aReference"))!.remove(); + + expect(computedTargetName.value).toBeUndefined(); + expect(holder.getReferenceTargetNode(toRoleJS("aReference"))).toBeNull(); +}); + +test("without the option, a reference to a deleted node keeps the deleted node", () => { + const { holder } = useModelWithReference({}); + const computedTargetNode = computed(() => + holder.getReferenceTargetNode(toRoleJS("aReference")), + ); + const target = computedTargetNode.value!; + + target.remove(); + + expect(computedTargetNode.value).toBe(target); +}); + +test("reference into a deleted subtree reads as unset", () => { + const { rootNode, holder } = useModelWithReference(); + const computedTargetNode = computed(() => + holder.getReferenceTargetNode(toRoleJS("aReference")), + ); + expect(computedTargetNode.value).not.toBeNull(); + + rootNode.getChildren(toRoleJS("containers"))[0].remove(); + + expect(computedTargetNode.value).toBeNull(); +}); + +test("without the option, nodes do not track their removal", () => { + const { holder } = useModelWithReference({}); + const target = holder.getReferenceTargetNode(toRoleJS("aReference"))!; + + expect(holder).not.toBeInstanceOf(RemovalTrackingReactiveINodeJS); + expect(target).not.toBeInstanceOf(RemovalTrackingReactiveINodeJS); + expect("refForRemoved" in target).toBe(false); +}); + +test("with the option, only targets of a read reference track their removal", () => { + const { holder, otherHolder } = useModelWithReference(); + const target = holder.getReferenceTargetNode(toRoleJS("aReference"))!; + + expect(target).toBeInstanceOf(RemovalTrackingReactiveINodeJS); + expect("refForRemoved" in target).toBe(true); + expect("refForRemoved" in otherHolder).toBe(false); +}); + +test("unrelated change to children does not update a valid reference", () => { + const { rootNode, holder } = useModelWithReference(); + let evaluations = 0; + const computedTargetNode = computed(() => { + evaluations++; + return holder.getReferenceTargetNode(toRoleJS("aReference")); + }); + const target = computedTargetNode.value; + expect(evaluations).toBe(1); + + rootNode.addNewChild(toRoleJS("holders"), -1, undefined).remove(); + + expect(computedTargetNode.value).toBe(target); + expect(evaluations).toBe(1); +}); + +test("removing a holder together with its target reads the references as unset", () => { + const rootNode = useModelsFromJson( + [ + JSON.stringify({ + root: { + children: [ + { + role: "holders", + references: { aReference: "target" }, + }, + { + role: "subtrees", + children: [ + { + role: "holders", + references: { aReference: "target" }, + }, + { id: "target", role: "items" }, + ], + }, + ], + }, + }), + ], + updateReferencesToRemovedNodes, + ); + const outerHolder = rootNode.getChildren(toRoleJS("holders"))[0]; + const subtree = rootNode.getChildren(toRoleJS("subtrees"))[0]; + const innerHolder = subtree.getChildren(toRoleJS("holders"))[0]; + const computedOuterTarget = computed(() => + outerHolder.getReferenceTargetNode(toRoleJS("aReference")), + ); + const computedInnerTarget = computed(() => + innerHolder.getReferenceTargetNode(toRoleJS("aReference")), + ); + expect(computedOuterTarget.value).not.toBeNull(); + expect(computedInnerTarget.value).toBe(computedOuterTarget.value); + + subtree.remove(); + + expect(computedOuterTarget.value).toBeNull(); + expect(computedInnerTarget.value).toBeNull(); +}); + +test("removing the former target of a reference does not update the reference", () => { + const { rootNode, holder } = useModelWithReference(); + const container = rootNode.getChildren(toRoleJS("containers"))[0]; + const oldTarget = container.getChildren(toRoleJS("items"))[0]; + const newTarget = container.addNewChild(toRoleJS("items"), -1, undefined); + let evaluations = 0; + const computedTargetNode = computed(() => { + evaluations++; + return holder.getReferenceTargetNode(toRoleJS("aReference")); + }); + expect(computedTargetNode.value).toBe(oldTarget); + holder.setReferenceTargetNode(toRoleJS("aReference"), newTarget); + expect(computedTargetNode.value).toBe(newTarget); + expect(evaluations).toBe(2); + + oldTarget.remove(); + + expect(computedTargetNode.value).toBe(newTarget); + expect(evaluations).toBe(2); + + newTarget.remove(); + + expect(computedTargetNode.value).toBeNull(); + expect(evaluations).toBe(3); +}); + +test("changes that neither add nor remove nodes do not update a valid reference", () => { + const { rootNode, holder, otherHolder } = useModelWithReference(); + let evaluations = 0; + const computedTargetNode = computed(() => { + evaluations++; + return holder.getReferenceTargetNode(toRoleJS("aReference")); + }); + const target = computedTargetNode.value; + + rootNode.moveChild(toRoleJS("holders"), -1, holder); + otherHolder.setPropertyValue(toRoleJS("name"), "renamed"); + otherHolder.setReferenceTargetNode(toRoleJS("aReference"), holder); + + expect(rootNode.getChildren(toRoleJS("holders"))).toEqual([ + otherHolder, + holder, + ]); + expect(computedTargetNode.value).toBe(target); + expect(evaluations).toBe(1); +}); + +test("moving the target of a reference does not update the reference", () => { + const { rootNode, holder } = useModelWithReference(); + let evaluations = 0; + const computedTargetNode = computed(() => { + evaluations++; + return holder.getReferenceTargetNode(toRoleJS("aReference")); + }); + const target = computedTargetNode.value!; + + rootNode.moveChild(toRoleJS("items"), -1, target); + + expect(rootNode.getChildren(toRoleJS("items"))).toEqual([target]); + expect(computedTargetNode.value).toBe(target); + expect(evaluations).toBe(1); +}); + +test("a reference read outside a computed reads as unset in a computed after its target is removed", () => { + const { holder } = useModelWithReference(); + holder.getReferenceTargetNode(toRoleJS("aReference"))!.remove(); + + const computedTargetNode = computed(() => + holder.getReferenceTargetNode(toRoleJS("aReference")), + ); + + expect(computedTargetNode.value).toBeNull(); +}); + +test("a new target that is removed before the reference is read again reads as unset", () => { + const { rootNode, holder } = useModelWithReference(); + const computedTargetNode = computed(() => + holder.getReferenceTargetNode(toRoleJS("aReference")), + ); + expect(computedTargetNode.value).not.toBeNull(); + const newTarget = rootNode.addNewChild(toRoleJS("items"), -1, undefined); + holder.setReferenceTargetNode(toRoleJS("aReference"), newTarget); + + // The new target was never read through the reference before its removal. + newTarget.remove(); + + expect(computedTargetNode.value).toBeNull(); +}); + +// The JS API cannot add a node with the reference of a removed node, as an undo does. +// So these tests use fake nodes and pass the changes to `handleChange` directly. +function useFakeModelWithReference() { + const nodesInModel = new Set(["target"]); + const target = { getReference: () => "target" } as unknown as INodeJS; + const holder = { + getReference: () => "holder", + getReferenceTargetNode: () => (nodesInModel.has("target") ? target : null), + } as unknown as INodeJS; + const cache = new RemovalTrackingCache(); + const reactiveHolder = toReactiveINodeJS(holder, cache); + const computedTargetReference = computed(() => + reactiveHolder + .getReferenceTargetNode(toRoleJS("aReference")) + ?.getReference(), + ); + function remove(node: INodeJS) { + nodesInModel.delete(node.getReference()); + handleAddedOrRemovedNode(new NodeRemoved(node), cache); + } + function add(node: INodeJS) { + nodesInModel.add(node.getReference()); + handleAddedOrRemovedNode(new NodeAdded(node), cache); + } + return { target, computedTargetReference, remove, add }; +} + +test("reference to a removed node resolves again when the node is added again", () => { + const { target, computedTargetReference, remove, add } = + useFakeModelWithReference(); + expect(computedTargetReference.value).toBe("target"); + + remove(target); + + expect(computedTargetReference.value).toBeUndefined(); + + add(target); + + expect(computedTargetReference.value).toBe("target"); +}); + +// Known limit: the reference is not updated, because the target node was never read through it. +test.failing( + "a reference first read while its target is removed resolves when the target is added again", + () => { + const { target, computedTargetReference, remove, add } = + useFakeModelWithReference(); + remove(target); + expect(computedTargetReference.value).toBeUndefined(); + + add(target); + + expect(computedTargetReference.value).toBe("target"); + }, +); diff --git a/vue-model-api/src/internal/ReactiveINodeJS.ts b/vue-model-api/src/internal/ReactiveINodeJS.ts index 20befe714e..9ecfe55647 100644 --- a/vue-model-api/src/internal/ReactiveINodeJS.ts +++ b/vue-model-api/src/internal/ReactiveINodeJS.ts @@ -7,7 +7,7 @@ import type { } from "@modelix/ts-model-api"; import type { Ref } from "vue"; import { markRaw, shallowRef } from "vue"; -import type { Cache } from "./Cache"; +import { Cache } from "./Cache"; import type { Nullable } from "@modelix/model-client"; import { org } from "@modelix/model-client"; @@ -64,7 +64,7 @@ export class ReactiveINodeJS implements INodeJS { constructor( public readonly unreactiveNode: INodeJS, - private readonly cache: Cache, + protected readonly cache: Cache, ) { // markRaw ensures, that Vue.js does not wrap this object again in a proxy // see. https://vuejs.org/api/reactivity-advanced.html#markraw @@ -77,7 +77,7 @@ export class ReactiveINodeJS implements INodeJS { private referenceTargetRefGetter = (role: ReferenceRole) => this.unreactiveNode.getReferenceTargetRef(role); - private referenceTargetNodeGetter = (role: ReferenceRole) => { + protected referenceTargetNodeGetter = (role: ReferenceRole) => { const unreactiveTargetNode = this.unreactiveNode.getReferenceTargetNode(role); return unreactiveTargetNode @@ -280,3 +280,63 @@ export class ReactiveINodeJS implements INodeJS { return this.unreactiveNode; } } + +/** + * A reactive node that reads a reference to a removed node as unset. + * It is only created with `updateReferencesToRemovedNodes`, + * so that the other reactive nodes cost the same as without this option. + */ +export class RemovalTrackingReactiveINodeJS extends ReactiveINodeJS { + // Whether this node was removed from the model. + // It is only created for nodes that are the target of a read reference. + // `declare` keeps it from being a field of every node. + declare private refForRemoved?: Ref; + + protected override referenceTargetNodeGetter = (role: ReferenceRole) => { + const unreactiveTargetNode = + this.unreactiveNode.getReferenceTargetNode(role); + if (!unreactiveTargetNode) { + return unreactiveTargetNode; + } + const targetNode = toReactiveINodeJS( + unreactiveTargetNode, + this.cache, + ) as RemovalTrackingReactiveINodeJS; + // The flag is created here, while the target node exists, and not when it is read. + // A dirty `computed` that is evaluated after the removal must not get a new `false`. + targetNode.refForRemoved ??= shallowRef(false); + return targetNode; + }; + + override getReferenceTargetNode(role: ReferenceRole): INodeJS | undefined { + const targetNode = super.getReferenceTargetNode(role) as + RemovalTrackingReactiveINodeJS | undefined; + // A removed node is not reported as a change of the references to it, + // so a reference to a removed node is read as unset here. + if (targetNode?.refForRemoved?.value) { + // Unset references are `null`, even though the declared type says `undefined`. + // See https://issues.modelix.org/issue/MODELIX-567/ + return null as unknown as undefined; + } + return targetNode; + } + + triggerChangeInRemoved(removed: boolean) { + if (this.refForRemoved) { + this.refForRemoved.value = removed; + } + } +} + +/** + * The cache for reactive nodes with `updateReferencesToRemovedNodes`. + * It creates a {@link RemovalTrackingReactiveINodeJS} for every node. + */ +export class RemovalTrackingCache extends Cache { + override memoize(node: INodeJS): RemovalTrackingReactiveINodeJS { + return super.memoize( + node, + () => new RemovalTrackingReactiveINodeJS(node, this), + ); + } +} diff --git a/vue-model-api/src/internal/handleChange.ts b/vue-model-api/src/internal/handleChange.ts index 31243cab10..bbd4aa1e6b 100644 --- a/vue-model-api/src/internal/handleChange.ts +++ b/vue-model-api/src/internal/handleChange.ts @@ -1,12 +1,59 @@ import { org } from "@modelix/model-client"; import type { ReactiveINodeJS } from "./ReactiveINodeJS"; +import { RemovalTrackingCache } from "./ReactiveINodeJS"; import type { Cache } from "./Cache"; import { toRoleJS } from "@modelix/ts-model-api"; -const { PropertyChanged, ReferenceChanged, ChildrenChanged } = - org.modelix.model.client2; +const { + PropertyChanged, + ReferenceChanged, + ChildrenChanged, + NodeAdded, + NodeRemoved, +} = org.modelix.model.client2; type ChangeJS = org.modelix.model.client2.ChangeJS; +type MutableModelTreeJs = org.modelix.model.client2.MutableModelTreeJs; + +// Without `updateReferencesToRemovedNodes`, `handler` is added with `addListener` as it is, +// so that changes cost the same as without this option. +export function addChangeHandler( + tree: MutableModelTreeJs, + cache: Cache, + handler: (change: ChangeJS) => void, +) { + if (cache instanceof RemovalTrackingCache) { + tree.addListenerIncludingAddedAndRemovedNodes((change: ChangeJS) => { + if (!handleAddedOrRemovedNode(change, cache)) { + handler(change); + } + }); + } else { + tree.addListener(handler); + } +} + +/** + * Handles `NodeAdded` and `NodeRemoved`, which are only reported with `updateReferencesToRemovedNodes`. + * + * @returns Whether `change` is one of them. + */ +export function handleAddedOrRemovedNode( + change: ChangeJS, + cache: RemovalTrackingCache, +): boolean { + const removed = change instanceof NodeRemoved; + if (!removed && !(change instanceof NodeAdded)) { + return false; + } + // A node can be added again with the same reference, for example, by an undo. + // Then a reference to it resolves again, + // but only if the reference was read while the node existed. + // Known limit: a reference that is first read, or changed, while the node is removed + // stays unset after the node is added again, until the reference changes again. + cache.get(change.node)?.triggerChangeInRemoved(removed); + return true; +} export function handleChange(change: ChangeJS, cache: Cache) { const unreactiveNode = change.node; diff --git a/vue-model-api/src/useModelsFromJson.ts b/vue-model-api/src/useModelsFromJson.ts index db0b9e988c..1bf9bbacc4 100644 --- a/vue-model-api/src/useModelsFromJson.ts +++ b/vue-model-api/src/useModelsFromJson.ts @@ -1,8 +1,12 @@ import type { INodeJS } from "@modelix/ts-model-api"; import type { ReactiveINodeJS } from "./internal/ReactiveINodeJS"; -import { toReactiveINodeJS } from "./internal/ReactiveINodeJS"; +import { + RemovalTrackingCache, + toReactiveINodeJS, +} from "./internal/ReactiveINodeJS"; import { Cache } from "./internal/Cache"; -import { handleChange } from "./internal/handleChange"; +import { addChangeHandler, handleChange } from "./internal/handleChange"; +import type { ReactiveModelOptions } from "./ReactiveModelOptions"; import { org } from "@modelix/model-client"; const { loadModelsFromJsonAsBranch } = org.modelix.model.client2; @@ -15,12 +19,20 @@ type ChangeJS = org.modelix.model.client2.ChangeJS; * The returned root node uses Vues reactivity and can be used in Vue like an reactive object. * * @param modelDataJsonStrings - Array of string, each representing a root node. + * @param options - Set `updateReferencesToRemovedNodes` to make `getReferenceTargetNode` read a reference + * to a removed node as unset, so that a view showing it updates. + * `getReferenceTargetRef` still returns the reference. It costs work for every added or removed node. * @returns A new root node the combines all children from the loaded root nodes. */ -export function useModelsFromJson(modelDataJsonStrings: string[]): INodeJS { - const cache = new Cache(); +export function useModelsFromJson( + modelDataJsonStrings: string[], + options?: ReactiveModelOptions, +): INodeJS { + const cache = options?.updateReferencesToRemovedNodes + ? new RemovalTrackingCache() + : new Cache(); const branch = loadModelsFromJsonAsBranch(modelDataJsonStrings); - branch.addListener((change: ChangeJS) => { + addChangeHandler(branch, cache, (change: ChangeJS) => { handleChange(change, cache); }); const reactiveRootNode = toReactiveINodeJS(branch.rootNode, cache); diff --git a/vue-model-api/src/useReplicatedModel.test.ts b/vue-model-api/src/useReplicatedModel.test.ts index 1e056be132..871a42c837 100644 --- a/vue-model-api/src/useReplicatedModel.test.ts +++ b/vue-model-api/src/useReplicatedModel.test.ts @@ -1,6 +1,7 @@ import { org } from "@modelix/model-client"; import { toRoleJS } from "@modelix/ts-model-api"; -import { watchEffect } from "vue"; +import type { Ref } from "vue"; +import { nextTick, ref, watchEffect } from "vue"; import { useModelClient } from "./useModelClient"; import { useReplicatedModel } from "./useReplicatedModel"; import IdSchemeJS = org.modelix.model.client2.IdSchemeJS; @@ -222,3 +223,89 @@ test("test wrapper backwards compatibility", (done) => { } }); }); + +test.each([ + { + options: undefined, + method: "addListener", + otherMethod: "addListenerIncludingAddedAndRemovedNodes", + }, + { + options: { updateReferencesToRemovedNodes: true }, + method: "addListenerIncludingAddedAndRemovedNodes", + otherMethod: "addListener", + }, +] as const)( + "with options $options, the change handler is added with $method", + async ({ options, method, otherMethod }) => { + const branch = { + rootNode: loadModelsFromJson([JSON.stringify({ root: {} })]), + addListener: jest.fn(), + addListenerIncludingAddedAndRemovedNodes: jest.fn(), + }; + const client = { + startReplicatedModel: () => Promise.resolve({ getBranch: () => branch }), + } as unknown as ClientJS; + + const { rootNode } = useReplicatedModel( + client, + "aRepository", + "aBranch", + IdSchemeJS.MODELIX, + options, + ); + await new Promise((resolve) => + watchEffect(() => { + if (rootNode.value !== null) { + resolve(); + } + }), + ); + + expect(branch[method]).toHaveBeenCalled(); + expect(branch[otherMethod]).not.toHaveBeenCalled(); + }, +); + +test("changing the options after the call does not change how a reconnect adds the change handler", async () => { + const branch = { + rootNode: loadModelsFromJson([JSON.stringify({ root: {} })]), + addListener: jest.fn(), + addListenerIncludingAddedAndRemovedNodes: jest.fn(), + }; + const client = { + startReplicatedModel: () => + Promise.resolve({ getBranch: () => branch, dispose: () => {} }), + } as unknown as ClientJS; + const branchId = ref("aBranch"); + const options = { updateReferencesToRemovedNodes: false }; + + const { rootNode } = useReplicatedModel( + client, + "aRepository", + branchId, + IdSchemeJS.MODELIX, + options, + ); + await waitUntilConnected(rootNode); + options.updateReferencesToRemovedNodes = true; + branchId.value = "anotherBranch"; + // Let the effect disconnect, before waiting for the reconnect. + await nextTick(); + await waitUntilConnected(rootNode); + + expect(branch.addListener).toHaveBeenCalledTimes(2); + expect( + branch.addListenerIncludingAddedAndRemovedNodes, + ).not.toHaveBeenCalled(); +}); + +function waitUntilConnected(rootNode: Ref) { + return new Promise((resolve) => + watchEffect(() => { + if (rootNode.value !== null) { + resolve(); + } + }), + ); +} diff --git a/vue-model-api/src/useReplicatedModel.ts b/vue-model-api/src/useReplicatedModel.ts index 4c42060578..d53251edb1 100644 --- a/vue-model-api/src/useReplicatedModel.ts +++ b/vue-model-api/src/useReplicatedModel.ts @@ -4,9 +4,13 @@ import { useLastPromiseEffect } from "./internal/useLastPromiseEffect"; import type { MaybeRefOrGetter, Ref } from "vue"; import { shallowRef, toValue } from "vue"; import type { ReactiveINodeJS } from "./internal/ReactiveINodeJS"; -import { toReactiveINodeJS } from "./internal/ReactiveINodeJS"; +import { + RemovalTrackingCache, + toReactiveINodeJS, +} from "./internal/ReactiveINodeJS"; import { Cache } from "./internal/Cache"; -import { handleChange } from "./internal/handleChange"; +import { addChangeHandler, handleChange } from "./internal/handleChange"; +import type { ReactiveModelOptions } from "./ReactiveModelOptions"; type ClientJS = org.modelix.model.client2.ClientJS; type ReplicatedModelJS = org.modelix.model.client2.ReplicatedModelJS; @@ -30,6 +34,9 @@ function isDefined(value: T | null | undefined): value is T { * @param client - Reactive reference of a client to a model server. * @param repositoryId - Reactive reference of a repositoryId on the model server. * @param branchId - Reactive reference of a branchId in the repository of the model server. + * @param options - Set `updateReferencesToRemovedNodes` to make `getReferenceTargetNode` read a reference + * to a removed node as unset, so that a view showing it updates. + * `getReferenceTargetRef` still returns the reference. It costs work for every added or removed node. * * @returns {Object} values Wrapper around different returned values. * @returns {Ref} values.rootNode Reactive reference to the replicated model for the specified branch. @@ -44,12 +51,17 @@ export function useReplicatedModel( idScheme: MaybeRefOrGetter< org.modelix.model.client2.IdSchemeJS | null | undefined >, + options?: ReactiveModelOptions, ): { replicatedModel: Ref; rootNode: Ref; dispose: () => void; error: Ref; } { + // Read once, so that changing `options` later does not change the behavior after a reconnect. + const updateReferencesToRemovedNodes = + options?.updateReferencesToRemovedNodes ?? false; + // Use `replicatedModel` to access the replicated model without tracking overhead of Vue.js. let replicatedModel: ReplicatedModelJS | null = null; const replicatedModelRef: Ref = shallowRef(null); @@ -86,7 +98,9 @@ export function useReplicatedModel( if (!isDefined(idSchemeValue)) { return; } - const cache = new Cache(); + const cache = updateReferencesToRemovedNodes + ? new RemovalTrackingCache() + : new Cache(); return clientValue .startReplicatedModel(repositoryIdValue, branchIdValue, idSchemeValue) .then((replicatedModel) => ({ replicatedModel, cache })); @@ -98,7 +112,7 @@ export function useReplicatedModel( if (isResultOfLastStartedPromise) { replicatedModel = connectedReplicatedModel; const branch = replicatedModel.getBranch(); - branch.addListener((change: ChangeJS) => { + addChangeHandler(branch, cache, (change: ChangeJS) => { if (cache === null) { throw Error("The cache is unexpectedly not set up."); } diff --git a/vue-model-api/src/useReplicatedModels.test.ts b/vue-model-api/src/useReplicatedModels.test.ts index d308f7c140..607665f4ff 100644 --- a/vue-model-api/src/useReplicatedModels.test.ts +++ b/vue-model-api/src/useReplicatedModels.test.ts @@ -31,6 +31,7 @@ class SuccessfulBranchJS { } addListener = jest.fn(); + addListenerIncludingAddedAndRemovedNodes = jest.fn(); } class SuccessfulReplicatedModelJS { @@ -267,3 +268,46 @@ test("rootNodes are returned in order and wrapped if readonly", (done) => { } }); }, 30000); + +test.each([ + { + options: undefined, + method: "addListener", + otherMethod: "addListenerIncludingAddedAndRemovedNodes", + }, + { + options: { updateReferencesToRemovedNodes: true }, + method: "addListenerIncludingAddedAndRemovedNodes", + otherMethod: "addListener", + }, +] as const)( + "with options $options, the change handler is added with $method", + async ({ options, method, otherMethod }) => { + const replicatedModel = new SuccessfulReplicatedModelJS("aBranch"); + const client = { + startReplicatedModels: () => Promise.resolve(replicatedModel), + } as unknown as ClientJS; + + const { rootNodes } = useReplicatedModels( + client, + [ + new ReplicatedModelParameters( + "aRepository", + "aBranch", + IdSchemeJS.MODELIX, + ), + ], + options, + ); + await new Promise((resolve) => + watchEffect(() => { + if (rootNodes.value.length > 0) { + resolve(); + } + }), + ); + + expect(replicatedModel.getBranch()[method]).toHaveBeenCalled(); + expect(replicatedModel.getBranch()[otherMethod]).not.toHaveBeenCalled(); + }, +); diff --git a/vue-model-api/src/useReplicatedModels.ts b/vue-model-api/src/useReplicatedModels.ts index 52b6d9a6cd..422f4d37a5 100644 --- a/vue-model-api/src/useReplicatedModels.ts +++ b/vue-model-api/src/useReplicatedModels.ts @@ -4,9 +4,13 @@ import { useLastPromiseEffect } from "./internal/useLastPromiseEffect"; import type { MaybeRefOrGetter, Ref } from "vue"; import { shallowRef, toValue } from "vue"; import type { ReactiveINodeJS } from "./internal/ReactiveINodeJS"; -import { toReactiveINodeJS } from "./internal/ReactiveINodeJS"; +import { + RemovalTrackingCache, + toReactiveINodeJS, +} from "./internal/ReactiveINodeJS"; import { Cache } from "./internal/Cache"; -import { handleChange } from "./internal/handleChange"; +import { addChangeHandler, handleChange } from "./internal/handleChange"; +import type { ReactiveModelOptions } from "./ReactiveModelOptions"; import { ReadOnlyNodeJS } from "./ReadonlyNodeJS"; type ClientJS = org.modelix.model.client2.ClientJS; @@ -32,6 +36,9 @@ function isDefined(value: T | null | undefined): value is T { * * @param client - Reactive reference of a client to a model server. * @param models - Reactive reference to an array of ReplicatedModelParameters. + * @param options - Set `updateReferencesToRemovedNodes` to make `getReferenceTargetNode` read a reference + * to a removed node as unset, so that a view showing it updates. + * `getReferenceTargetRef` still returns the reference. It costs work for every added or removed node. * * @returns {Object} values Wrapper around different returned values. * @returns {Ref} values.replicatedModel Reactive reference to the replicated model for the specified branches. @@ -42,12 +49,17 @@ function isDefined(value: T | null | undefined): value is T { export function useReplicatedModels( client: MaybeRefOrGetter, models: MaybeRefOrGetter, + options?: ReactiveModelOptions, ): { replicatedModel: Ref; rootNodes: Ref; dispose: () => void; error: Ref; } { + // Read once, so that changing `options` later does not change the behavior after a reconnect. + const updateReferencesToRemovedNodes = + options?.updateReferencesToRemovedNodes ?? false; + // Use `replicatedModel` to access the replicated model without tracking overhead of Vue.js. let replicatedModel: ReplicatedModelJS | null = null; const replicatedModelRef: Ref = shallowRef(null); @@ -80,7 +92,9 @@ export function useReplicatedModels( if (!isDefined(modelsValue)) { return; } - const cache = new Cache(); + const cache = updateReferencesToRemovedNodes + ? new RemovalTrackingCache() + : new Cache(); return clientValue .startReplicatedModels(modelsValue) .then((replicatedModel) => ({ @@ -96,7 +110,7 @@ export function useReplicatedModels( if (isResultOfLastStartedPromise) { replicatedModel = connectedReplicatedModel; const branch = replicatedModel.getBranch(); - branch.addListener((change: ChangeJS) => { + addChangeHandler(branch, cache, (change: ChangeJS) => { if (cache === null) { throw Error("The cache is unexpectedly not set up."); }