Fix node removing bug, caused by spurious watchEffect effect. (#11099)

Fixes [#11062](#11062)

Modified watchEffect to be consistent with Vue behavior + tests. Before the scheduled effects were run even if stopped.

Why it fixes nodes removal? See [this comment](#11062 (comment))

app/gui2/e2e/removingNodes.spec.ts

@@ -0,0 +1,54 @@
+import { test } from '@playwright/test'
+import * as actions from './actions'
+import { expect } from './customExpect'
+import { CONTROL_KEY, DELETE_KEY } from './keyboard'
+import * as locate from './locate'
+
+test('Deleting selected node with backspace key', async ({ page }) => {
+  await actions.goToGraph(page)
+
+  const nodesCount = await locate.graphNode(page).count()
+  const deletedNode = locate.graphNodeByBinding(page, 'final')
+  await deletedNode.click()
+  await page.keyboard.press('Backspace')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1)
+})
+
+test('Deleting selected node with delete key', async ({ page }) => {
+  await actions.goToGraph(page)
+
+  const nodesCount = await locate.graphNode(page).count()
+  const deletedNode = locate.graphNodeByBinding(page, 'final')
+  await deletedNode.click()
+  await page.keyboard.press('Delete')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1)
+})
+
+test('Graph can be empty', async ({ page }) => {
+  await actions.goToGraph(page)
+
+  await locate.graphEditor(page).press(`${CONTROL_KEY}+A`)
+  await locate.graphEditor(page).press(`${DELETE_KEY}`)
+
+  await expect(locate.graphNode(page)).toHaveCount(0)
+
+  await locate.addNewNodeButton(page).click()
+  await expect(locate.componentBrowserInput(page)).toBeVisible()
+  await page.keyboard.insertText('foo')
+  await page.keyboard.press(`${CONTROL_KEY}+Enter`)
+  await expect(locate.graphNode(page)).toHaveCount(1)
+  await expect(locate.graphNode(page).locator('.WidgetToken')).toHaveText(['foo'])
+})
+
+test('Removing connected nodes', async ({ page }) => {
+  await actions.goToGraph(page)
+  const nodesCount = await locate.graphNode(page).count()
+  await page.keyboard.down('Shift')
+  await locate.graphNodeByBinding(page, 'five').click()
+  await expect(locate.selectedNodes(page)).toHaveCount(1)
+  await locate.graphNodeByBinding(page, 'sum').click()
+  await expect(locate.selectedNodes(page)).toHaveCount(2)
+  await page.keyboard.up('Shift')
+  await page.keyboard.press('Delete')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount - 2)
+})

app/gui2/e2e/selectingNodes.spec.ts

@@ -68,26 +68,6 @@ test('Selecting nodes by area drag', async ({ page }) => {
   await expect(node2).toBeSelected()
 })
 
-test('Deleting selected node with backspace key', async ({ page }) => {
-  await actions.goToGraph(page)
-
-  const nodesCount = await locate.graphNode(page).count()
-  const deletedNode = locate.graphNodeByBinding(page, 'final')
-  await deletedNode.click()
-  await page.keyboard.press('Backspace')
-  await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1)
-})
-
-test('Deleting selected node with delete key', async ({ page }) => {
-  await actions.goToGraph(page)
-
-  const nodesCount = await locate.graphNode(page).count()
-  const deletedNode = locate.graphNodeByBinding(page, 'final')
-  await deletedNode.click()
-  await page.keyboard.press('Delete')
-  await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1)
-})
-
 test('Moving selected nodes', async ({ page }) => {
   await actions.goToGraph(page)
   const movedNode = locate.graphNodeByBinding(page, 'final')

app/gui2/src/util/__tests__/reactivity.test.ts

@@ -1,7 +1,7 @@
-import { LazySyncEffectSet, syncSet } from '@/util/reactivity'
+import { LazySyncEffectSet, syncSet, useWatchContext } from '@/util/reactivity'
 import { fc, test } from '@fast-check/vitest'
-import { expect, vi } from 'vitest'
-import { nextTick, reactive, ref } from 'vue'
+import { describe, expect, vi } from 'vitest'
+import { nextTick, reactive, ref, watchEffect, WatchStopHandle } from 'vue'
 
 test('LazySyncEffectSet', async () => {
   const lazySet = new LazySyncEffectSet()
@@ -75,9 +75,6 @@ test('LazySyncEffectSet', async () => {
   key2.value = 104
   lazySet.lazyEffect((onCleanup) => {
     const currentValue = key1.value
-    console.log('currentValue', currentValue)
-    console.log('lazilyUpdatedMap', lazilyUpdatedMap)
-
     lazilyUpdatedMap.set(currentValue, 'c' + runCount++)
     onCleanup(() => lazilyUpdatedMap.delete(currentValue))
   })
@@ -154,3 +151,30 @@ test.prop({
   syncSet(target, newState)
   expect([...target].sort()).toEqual([...newState].sort())
 })
+
+describe('Stopping WatchEffect prevents already scheduled effects:', () => {
+  async function testProper(watchEffect: (f: () => void) => WatchStopHandle) {
+    const dep = ref(false)
+    const f = vi.fn()
+    const stop = watchEffect(() => {
+      if (dep.value) {
+        f()
+      }
+    })
+    expect(f).not.toHaveBeenCalled()
+    dep.value = true
+    stop()
+    await nextTick()
+    expect(f).not.toHaveBeenCalled()
+  }
+
+  test('from useWatchContext', async () => {
+    const ctx = useWatchContext()
+    await testProper(ctx.watchEffect)
+  })
+
+  // We check if we're consistent with Vue API.
+  test('original from Vue', async () => {
+    await testProper(watchEffect)
+  })
+})

app/gui2/src/util/reactivity.ts

@@ -152,8 +152,10 @@ export function useWatchContext(): { watchEffect: (f: () => void) => WatchStopHa
   watch(jobs, () => {
     while (jobs.length > 0) {
       const job = jobs.pop()!
-      queued.delete(job)
-      job()
+      // Do not run scheduled job if it's stopped. It's consistent with vue's "watchEffect" (checked in tests)
+      if (queued.delete(job)) {
+        job()
+      }
     }
   })
   function watchEffect(f: () => void) {
@@ -166,7 +168,10 @@ export function useWatchContext(): { watchEffect: (f: () => void) => WatchStopHa
       },
       allowRecurse: true,
     })
-    return runner.effect.stop.bind(runner.effect)
+    return () => {
+      runner.effect.stop()
+      queued.delete(runner)
+    }
   }
   return { watchEffect }
 }