fix(reactivity): avoid error during nested watch stop

[edison1105]

fix #5783
this because effects may be changed when an effect stoped.

[netlify]

❌ Deploy Preview for vuejs-coverage failed.

Name	Link
🔨 Latest commit	7b1c191
🔍 Latest deploy log	https://app.netlify.com/sites/vuejs-coverage/deploys/6296bb411289c000084714ec

[netlify]

✅ Deploy Preview for vue-sfc-playground ready!

Name	Link
🔨 Latest commit	5164e99
🔍 Latest deploy log	https://app.netlify.com/sites/vue-sfc-playground/deploys/6268d6d2b74d8e000861ac3e
😎 Deploy Preview	https://deploy-preview-5806--vue-sfc-playground.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[netlify]

✅ Deploy Preview for vue-next-template-explorer ready!

Name	Link
🔨 Latest commit	5164e99
🔍 Latest deploy log	https://app.netlify.com/sites/vue-next-template-explorer/deploys/6268d6d20d72cc000a13bf59
😎 Deploy Preview	https://deploy-preview-5806--vue-next-template-explorer.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[liulinboyi]

@edison1105
Because of this, I feel this can use:

while (this.effects.length) {
    this.effects[0].stop()
}

[edison1105]

@edison1105 Because of this, I feel this can use:

while (this.effects.length) {
    this.effects[0].stop()
}

but, Not every effect.stop() will change the effects

[liulinboyi]

@edison1105 Because of this, I feel this can use:

while (this.effects.length) {
    this.effects[0].stop()
}

but, Not every effect.stop() will change the effects

You are right, let me have a think other methods.

[liulinboyi]

@edison1105 Because of this, I feel this can use:

while (this.effects.length) {
    this.effects[0].stop()
}

but, Not every effect.stop() will change the effects

You are right, let me have a think other methods.

I feel this can use

const effects = this.effects ? [...this.effects] : []
for (i = 0, l = effects.length; i < l; i++) {
   effects[i].stop()
}

or

const effects = this.effects ? [...this.effects] : []
effects.forEach(e => e.stop())

or

(this.effects ? [...this.effects] : []).forEach(e => e.stop())

[github-actions]

Size Report

Bundles

File	Size	Gzip	Brotli
runtime-dom.global.prod.js	98.7 kB (+5 B)	37.4 kB (+8 B)	33.7 kB (+17 B)
vue.global.prod.js	156 kB (+5 B)	57.2 kB (+7 B)	50.9 kB (+110 B)

Usages

Name	Size	Gzip	Brotli
createApp	54.2 kB (+5 B)	21 kB (+7 B)	19.1 kB (+6 B)
createSSRApp	58.1 kB (+5 B)	22.6 kB (+9 B)	20.6 kB (-5 B)
defineCustomElement	58.8 kB (+5 B)	22.5 kB (+6 B)	20.5 kB (+64 B)
overall	67.8 kB (+5 B)	26 kB (+9 B)	23.7 kB (-34 B)

[skirtles-code]

Overall I think this is a good change, but I'm not sure about the test.

I did an experiment where I 'fixed' the problem with the following crude hack:

      for (i = 0, l = this.effects.length; i < l; i++) {
        if (this.effects[i]) {
          this.effects[i].stop()
        }
      }

I don't think this is the right fix, but the proposed test does still pass with this code. I feel like there should be a test that fails with my dodgy fix.

[skirtles-code]

I still think there's a problem with the testing here.

The test confirms that stopping the effectScope doesn't throw an error, but it doesn't confirm that the watchers actually get stopped.

The current code in this PR is:

const effectsToStop = this.effects.slice()
for (i = 0, l = effectsToStop.length; i < l; i++) {
  effectsToStop[i].stop()
}

I believe this does stop the watchers correctly, but the test doesn't really check that.

Consider the code below. All it does is introduce an if check, rather than copying the array. This code also fixes the original error and passes the tests:

for (i = 0, l = this.effects.length; i < l; i++) {
  // This 'fix' would be incorrect, but it looks plausible
  if (this.effects[i]) {
    this.effects[i].stop()
  }
}

The code above looks like a plausible fix, but it's actually flawed. If an earlier effect is removed while the loop is being processed it will cause the next effect to be skipped.

Currently we don't have a test to protect against an incorrect implementation like this.

The test below would catch that problem. It passes for the change proposed on this PR but fails (correctly) for the incorrect fix I showed above:

  test('removing a watcher while stopping its effectScope', async () => {
    const count = ref(0)
    const scope = effectScope()
    let watcherCalls = 0
    let cleanupCalls = 0

    scope.run(() => {
      const stop1 = watch(count, () => {
        watcherCalls++
      })
      watch(count, (val, old, onCleanup) => {
        watcherCalls++
        onCleanup(() => {
          cleanupCalls++
          stop1()
        })
      })
      watch(count, () => {
        watcherCalls++
      })
    })

    expect(watcherCalls).toBe(0)
    expect(cleanupCalls).toBe(0)

    count.value++
    await nextTick()
    expect(watcherCalls).toBe(3)
    expect(cleanupCalls).toBe(0)

    scope.stop()
    count.value++
    await nextTick()
    expect(watcherCalls).toBe(3)
    expect(cleanupCalls).toBe(1)
  })

[edison1105]

@skirtles-code
Thank you so much.

[yyx990803]

2193284 avoids the extra array copy and also avoid unnecessary remove call when the effectScope is already stopped.