Skip to content

Commit

Permalink
Refactor watch sources for reliability
Browse files Browse the repository at this point in the history
This commit changes `WatchSource` signatures into `Readonly<Ref>`s.

It provides two important benefits:

1. Eliminates the possibility of `undefined` states, that's result of
   using `WatchSource`s. This previously required additional null checks.
   By using `Readonly<Ref>`, the state handling becomes simpler and less
   susceptible to null errors.
2. Optimizes performance by using references:
   - Avoids the reactive layer of `computed` references when not needed.
   - The `watch` syntax, such as `watch(() => ref.value)`, can introduce
     side effects. For example, it does not account for `triggerRef` in
     scenarios where the value remains unchanged, preventing the watcher
     from running (vuejs/core#9579).
  • Loading branch information
undergroundwires committed Nov 11, 2023
1 parent 58cd551 commit 7ab16ec
Show file tree
Hide file tree
Showing 25 changed files with 190 additions and 217 deletions.
4 changes: 2 additions & 2 deletions src/presentation/components/Scripts/View/Tree/ScriptsTree.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
</template>

<script lang="ts">
import { defineComponent } from 'vue';
import { defineComponent, toRef } from 'vue';
import { injectKey } from '@/presentation/injectionSymbols';
import TreeView from './TreeView/TreeView.vue';
import NodeContent from './NodeContent/NodeContent.vue';
Expand All @@ -42,7 +42,7 @@ export default defineComponent({
const useUserCollectionStateHook = injectKey((keys) => keys.useUserSelectionState);
const { selectedScriptNodeIds } = useSelectedScriptNodeIds(useUserCollectionStateHook);
const { latestFilterEvent } = useTreeViewFilterEvent();
const { treeViewInputNodes } = useTreeViewNodeInput(() => props.categoryId);
const { treeViewInputNodes } = useTreeViewNodeInput(toRef(props, 'categoryId'));
const { updateNodeSelection } = useCollectionSelectionStateUpdater(useUserCollectionStateHook);
function handleNodeChangedEvent(event: TreeNodeStateChangedEmittedEvent) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<div class="wrapper" v-if="currentNode">
<div class="wrapper">
<div
class="expansible-node"
@click="toggleCheck"
Expand Down Expand Up @@ -46,15 +46,14 @@
</template>

<script lang="ts">
import {
defineComponent, computed, PropType,
} from 'vue';
import { defineComponent, computed, toRef } from 'vue';
import { TreeRoot } from '../TreeRoot/TreeRoot';
import { useCurrentTreeNodes } from '../UseCurrentTreeNodes';
import { NodeRenderingStrategy } from '../Rendering/Scheduling/NodeRenderingStrategy';
import { useNodeState } from './UseNodeState';
import { TreeNode } from './TreeNode';
import LeafTreeNode from './LeafTreeNode.vue';
import type { PropType } from 'vue';
export default defineComponent({
name: 'HierarchicalTreeNode', // Needed due to recursion
Expand All @@ -76,33 +75,32 @@ export default defineComponent({
},
},
setup(props) {
const { nodes } = useCurrentTreeNodes(() => props.treeRoot);
const currentNode = computed<TreeNode | undefined>(
() => nodes.value?.getNodeById(props.nodeId),
const { nodes } = useCurrentTreeNodes(toRef(props, 'treeRoot'));
const currentNode = computed<TreeNode>(
() => nodes.value.getNodeById(props.nodeId),
);
const { state } = useNodeState(() => currentNode.value);
const expanded = computed<boolean>(() => state.value?.isExpanded ?? false);
const { state } = useNodeState(currentNode);
const expanded = computed<boolean>(() => state.value.isExpanded);
const renderedNodeIds = computed<readonly string[]>(
() => currentNode.value
?.hierarchy
.hierarchy
.children
.filter((child) => props.renderingStrategy.shouldRender(child))
.map((child) => child.id)
?? [],
.map((child) => child.id),
);
function toggleExpand() {
currentNode.value?.state.toggleExpand();
currentNode.value.state.toggleExpand();
}
function toggleCheck() {
currentNode.value?.state.toggleCheck();
currentNode.value.state.toggleCheck();
}
const hasChildren = computed<boolean>(
() => currentNode.value?.hierarchy.isBranchNode,
() => currentNode.value.hierarchy.isBranchNode,
);
return {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
<template>
<li
v-if="currentNode"
class="wrapper"
@click.stop="toggleCheckState"
>
Expand Down Expand Up @@ -31,15 +30,14 @@
</template>

<script lang="ts">
import {
defineComponent, computed, PropType,
} from 'vue';
import { defineComponent, computed, toRef } from 'vue';
import { TreeRoot } from '../TreeRoot/TreeRoot';
import { useCurrentTreeNodes } from '../UseCurrentTreeNodes';
import { useNodeState } from './UseNodeState';
import { useKeyboardInteractionState } from './UseKeyboardInteractionState';
import { TreeNode } from './TreeNode';
import { TreeNodeCheckState } from './State/CheckState';
import type { PropType } from 'vue';
export default defineComponent({
props: {
Expand All @@ -54,16 +52,16 @@ export default defineComponent({
},
setup(props) {
const { isKeyboardBeingUsed } = useKeyboardInteractionState();
const { nodes } = useCurrentTreeNodes(() => props.treeRoot);
const currentNode = computed<TreeNode | undefined>(
() => nodes.value?.getNodeById(props.nodeId),
const { nodes } = useCurrentTreeNodes(toRef(props, 'treeRoot'));
const currentNode = computed<TreeNode>(
() => nodes.value.getNodeById(props.nodeId),
);
const { state } = useNodeState(() => currentNode.value);
const { state } = useNodeState(currentNode);
const hasFocus = computed<boolean>(() => state.value?.isFocused ?? false);
const checked = computed<boolean>(() => state.value?.checkState === TreeNodeCheckState.Checked);
const hasFocus = computed<boolean>(() => state.value.isFocused);
const checked = computed<boolean>(() => state.value.checkState === TreeNodeCheckState.Checked);
const indeterminate = computed<boolean>(
() => state.value?.checkState === TreeNodeCheckState.Indeterminate,
() => state.value.checkState === TreeNodeCheckState.Indeterminate,
);
const hasKeyboardFocus = computed<boolean>(() => {
Expand All @@ -74,14 +72,11 @@ export default defineComponent({
});
const onNodeFocus = () => {
if (!currentNode.value) {
return;
}
props.treeRoot.focus.setSingleFocus(currentNode.value);
};
function toggleCheckState() {
currentNode.value?.state.toggleCheck();
currentNode.value.state.toggleCheck();
}
return {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
import {
WatchSource, shallowRef, watch,
type Ref, shallowRef, watch, shallowReadonly,
} from 'vue';
import { injectKey } from '@/presentation/injectionSymbols';
import { ReadOnlyTreeNode } from './TreeNode';
import { TreeNodeStateDescriptor } from './State/StateDescriptor';

export function useNodeState(
nodeWatcher: WatchSource<ReadOnlyTreeNode | undefined>,
nodeRef: Readonly<Ref<ReadOnlyTreeNode>>,
) {
const { events } = injectKey((keys) => keys.useAutoUnsubscribedEvents);

const state = shallowRef<TreeNodeStateDescriptor>();
const state = shallowRef<TreeNodeStateDescriptor>(nodeRef.value.state.current);

watch(nodeWatcher, (node: ReadOnlyTreeNode) => {
if (!node) {
return;
}
watch(nodeRef, (node: ReadOnlyTreeNode) => {
state.value = node.state.current;
events.unsubscribeAllAndRegister([
node.state.changed.on((change) => {
Expand All @@ -25,6 +22,6 @@ export function useNodeState(
}, { immediate: true });

return {
state,
state: shallowReadonly(state),
};
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {
WatchSource, shallowRef, triggerRef, watch,
type Ref, shallowRef, triggerRef, watch,
} from 'vue';
import { ReadOnlyTreeNode } from '../Node/TreeNode';
import { useNodeStateChangeAggregator } from '../UseNodeStateChangeAggregator';
Expand All @@ -15,7 +15,7 @@ import { CollapsedParentOrderer } from './Ordering/CollapsedParentOrderer';
* Renders tree nodes gradually to prevent UI freeze when loading large amounts of nodes.
*/
export function useGradualNodeRendering(
treeWatcher: WatchSource<TreeRoot>,
treeRootRef: Readonly<Ref<TreeRoot>>,
useChangeAggregator = useNodeStateChangeAggregator,
useTreeNodes = useCurrentTreeNodes,
scheduler: DelayScheduler = new TimeoutDelayScheduler(),
Expand All @@ -28,8 +28,8 @@ export function useGradualNodeRendering(
let isRenderingInProgress = false;
const renderingDelayInMs = 50;

const { onNodeStateChange } = useChangeAggregator(treeWatcher);
const { nodes } = useTreeNodes(treeWatcher);
const { onNodeStateChange } = useChangeAggregator(treeRootRef);
const { nodes } = useTreeNodes(treeRootRef);

function updateNodeRenderQueue(node: ReadOnlyTreeNode, isVisible: boolean) {
if (isVisible
Expand All @@ -48,7 +48,7 @@ export function useGradualNodeRendering(
}
}

watch(() => nodes.value, (newNodes) => {
watch(nodes, (newNodes) => {
nodesToRender.clear();
nodesBeingRendered.value.clear();
if (!newNodes || newNodes.flattenedNodes.length === 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@

<script lang="ts">
import {
defineComponent, computed, PropType,
defineComponent, computed, toRef,
} from 'vue';
import HierarchicalTreeNode from '../Node/HierarchicalTreeNode.vue';
import { useCurrentTreeNodes } from '../UseCurrentTreeNodes';
import { NodeRenderingStrategy } from '../Rendering/Scheduling/NodeRenderingStrategy';
import { TreeRoot } from './TreeRoot';
import type { PropType } from 'vue';
export default defineComponent({
components: {
Expand All @@ -40,7 +41,7 @@ export default defineComponent({
},
},
setup(props) {
const { nodes } = useCurrentTreeNodes(() => props.treeRoot);
const { nodes } = useCurrentTreeNodes(toRef(props, 'treeRoot'));
const renderedNodeIds = computed<string[]>(() => {
return nodes
Expand Down
22 changes: 11 additions & 11 deletions src/presentation/components/Scripts/View/Tree/TreeView/TreeView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<script lang="ts">
import {
defineComponent, onMounted, watch,
shallowRef, PropType,
shallowRef, toRef, shallowReadonly,
} from 'vue';
import { TreeRootManager } from './TreeRoot/TreeRootManager';
import TreeRoot from './TreeRoot/TreeRoot.vue';
Expand All @@ -28,6 +28,7 @@ import { TreeNodeStateChangedEmittedEvent } from './Bindings/TreeNodeStateChange
import { useAutoUpdateParentCheckState } from './UseAutoUpdateParentCheckState';
import { useAutoUpdateChildrenCheckState } from './UseAutoUpdateChildrenCheckState';
import { useGradualNodeRendering } from './Rendering/UseGradualNodeRendering';
import type { PropType } from 'vue';
export default defineComponent({
components: {
Expand Down Expand Up @@ -57,17 +58,16 @@ export default defineComponent({
const tree = new TreeRootManager();
useTreeKeyboardNavigation(tree, treeContainerElement);
useTreeQueryFilter(
() => props.latestFilterEvent,
() => tree,
);
useLeafNodeCheckedStateUpdater(() => tree, () => props.selectedLeafNodeIds);
useAutoUpdateParentCheckState(() => tree);
useAutoUpdateChildrenCheckState(() => tree);
const nodeRenderingScheduler = useGradualNodeRendering(() => tree);
const treeRef = shallowReadonly(shallowRef(tree));
const { onNodeStateChange } = useNodeStateChangeAggregator(() => tree);
useTreeKeyboardNavigation(treeRef, treeContainerElement);
useTreeQueryFilter(toRef(props, 'latestFilterEvent'), treeRef);
useLeafNodeCheckedStateUpdater(treeRef, toRef(props, 'selectedLeafNodeIds'));
useAutoUpdateParentCheckState(treeRef);
useAutoUpdateChildrenCheckState(treeRef);
const nodeRenderingScheduler = useGradualNodeRendering(treeRef);
const { onNodeStateChange } = useNodeStateChangeAggregator(treeRef);
onNodeStateChange((change) => {
emit('nodeStateChanged', {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { WatchSource } from 'vue';
import { TreeRoot } from './TreeRoot/TreeRoot';
import { useNodeStateChangeAggregator } from './UseNodeStateChangeAggregator';
import { HierarchyAccess } from './Node/Hierarchy/HierarchyAccess';
import { TreeNodeCheckState } from './Node/State/CheckState';
import type { Ref } from 'vue';

export function useAutoUpdateChildrenCheckState(
treeWatcher: WatchSource<TreeRoot>,
treeRootRef: Readonly<Ref<TreeRoot>>,
useChangeAggregator = useNodeStateChangeAggregator,
) {
const { onNodeStateChange } = useChangeAggregator(treeWatcher);
const { onNodeStateChange } = useChangeAggregator(treeRootRef);

onNodeStateChange((change) => {
if (change.newState.checkState === change.oldState?.checkState) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import { WatchSource } from 'vue';
import { TreeRoot } from './TreeRoot/TreeRoot';
import { useNodeStateChangeAggregator } from './UseNodeStateChangeAggregator';
import { HierarchyAccess } from './Node/Hierarchy/HierarchyAccess';
import { TreeNodeCheckState } from './Node/State/CheckState';
import { ReadOnlyTreeNode } from './Node/TreeNode';
import type { Ref } from 'vue';

export function useAutoUpdateParentCheckState(
treeWatcher: WatchSource<TreeRoot>,
treeRef: Readonly<Ref<TreeRoot>>,
useChangeAggregator = useNodeStateChangeAggregator,
) {
const { onNodeStateChange } = useChangeAggregator(treeWatcher);
const { onNodeStateChange } = useChangeAggregator(treeRef);

onNodeStateChange((change) => {
if (change.newState.checkState === change.oldState?.checkState) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
import {
WatchSource, watch, shallowReadonly, shallowRef,
watch, shallowReadonly, shallowRef, type Ref,
} from 'vue';
import { injectKey } from '@/presentation/injectionSymbols';
import { TreeRoot } from './TreeRoot/TreeRoot';
import { QueryableNodes } from './TreeRoot/NodeCollection/Query/QueryableNodes';

export function useCurrentTreeNodes(treeWatcher: WatchSource<TreeRoot>) {
export function useCurrentTreeNodes(treeRef: Readonly<Ref<TreeRoot>>) {
const { events } = injectKey((keys) => keys.useAutoUnsubscribedEvents);

const tree = shallowRef<TreeRoot | undefined>();
const nodes = shallowRef<QueryableNodes | undefined>();
const nodes = shallowRef<QueryableNodes>(treeRef.value.collection.nodes);

watch(treeWatcher, (newTree) => {
tree.value = newTree;
watch(treeRef, (newTree) => {
nodes.value = newTree.collection.nodes;
events.unsubscribeAllAndRegister([
newTree.collection.nodesUpdated.on((newNodes) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import { WatchSource, watch } from 'vue';
import { type Ref, watch } from 'vue';
import { TreeRoot } from './TreeRoot/TreeRoot';
import { TreeNode } from './Node/TreeNode';
import { QueryableNodes } from './TreeRoot/NodeCollection/Query/QueryableNodes';
import { useCurrentTreeNodes } from './UseCurrentTreeNodes';
import { TreeNodeCheckState } from './Node/State/CheckState';

export function useLeafNodeCheckedStateUpdater(
treeWatcher: WatchSource<TreeRoot>,
leafNodeIdsWatcher: WatchSource<readonly string[]>,
treeRootRef: Readonly<Ref<TreeRoot>>,
leafNodeIdsRef: Readonly<Ref<readonly string[]>>,
) {
const { nodes } = useCurrentTreeNodes(treeWatcher);
const { nodes } = useCurrentTreeNodes(treeRootRef);

watch(
[leafNodeIdsWatcher, () => nodes.value],
[leafNodeIdsRef, nodes],
([nodeIds, actualNodes]) => {
updateNodeSelections(actualNodes, nodeIds);
},
Expand Down
Loading

0 comments on commit 7ab16ec

Please sign in to comment.