fix(reactivity): some mutation methods of Array cause infinite recursion

[unbyte]

Array.prototype.push, Array.prototype.pop, Array.prototype.unshift, Array.prototype.shift all access .length, which leads to an infinite recursion in some usage scenarios.

e.g.

const arr = reactive([])

watchEffect(()=>{
  arr.push(1)
})

watchEffect(()=>{
  arr.push(2)
})

close #2137

[edison1105]

this name doesn't make sence and I think we should use makeMap instead of new Set here.

[yyx990803]

I updated the implementation to use static instrumentations in the same map (this is more efficient than creating a new function on every access).

[CyberAP]

I am a bit confused by this change, a more common use-case is broken now:

  setup() { 
    const arr = ref([])

    watchEffect(() => {
      console.log('Array: ', arr.value)
    })

    arr.value.push('test') // doesn't log

    return { arr }
  }

[CyberAP]

I believe that by fixing the logic of these mutational effects it could be possible to solve the issue without affecting valid use-cases like above:

const arr = reactive([])

watchEffect(()=>{
  if (arr.includes(1)) return
  arr.push(1)
})

watchEffect(()=>{
  if (arr.includes(2)) return
  arr.push(2)
})

It is unclear why these effects should stop after the first run.

[edison1105]

I believe that by fixing the logic of these mutational effects it could be possible to solve the issue without affecting valid use-cases like above:

const arr = reactive([])

watchEffect(()=>{
  if (arr.includes(1)) return
  arr.push(1)
})

watchEffect(()=>{
  if (arr.includes(2)) return
  arr.push(2)
})

It is unclear why these effects should stop after the first run.

if we want arr = [1,1,1,1,1] ，what should we do ?

[CyberAP]

It depends on the logic that describes why it should call arr.push in the first place. That logic should be contained within the effect.
When I see an effect that says arr.push I think of it as this: when an array changes push another value there. But this is recursive by definition. So in order to break infinite cycle there should be a condition that properly describes under which circumstances arr.push should be called.

[unbyte]

I am a bit confused by this change, a more common use-case is broken now:

  setup() { 
    const arr = ref([])

    watchEffect(() => {
      console.log('Array: ', arr.value)
    })

    arr.value.push('test') // doesn't log

    return { arr }
  }

tested on rc.11 and rc.12, failed too

[CyberAP]

It works in Vue 2: https://jsfiddle.net/7bahj9su/
But does not work in Vue 3: https://jsfiddle.net/7bahj9su/1/

This change significantly diminishes profits of the new reactivity. While before you have had to use Vue.set to force reactivity to trigger, now you don't have to do anything. But with this change that's not true anymore, since you'll have to apply a hack for this to work properly:

watchEffect(() => {
  arr.value.length; // register a dependency
  console.log(`array: `, arr.value);
})

[yangmingshan]

@CyberAP I don't think your issue is related to this PR.

For JSFiddle example, it looks like a bug, and it also existed in previous RC versions.

For watchEffect example, it is expected behavior, since you only accessed value field, so Vue will only track value field changes, you may want write like this:

const arr = reactive([])

watch(arr, () => {
  console.log('Array: ', arr) // works as you expected.
})

[unbyte]

@yangmingshan It's not a bug cuz these platforms rewrite console.log so .length is accessed inside

[unbyte]

@CyberAP IMO it's expected cuz only arr.value doesn't declare any key of array as dep explicitly

[CyberAP]

For watchEffect example, it is expected behavior, since you only accessed value field, so Vue will only track value field changes, you may want write like this:

It is not the expected behaviour. When I am logging array.value I want this effect to be run every time the value of array changes. When I push into array the value of array does change, but the effect doesn't run. Right now you'll have to explicitly tell Vue that the value did change array.value = newArray, which removes the benefits of the new Proxy-based mutable reactivity that is extensively used in options API for example.

Your example doesn't work with ref, but it should work the same way it works with reactive. With reactive your array value changes when you push into that array, so why it shouldn't work the same way for ref value?

[yangmingshan]

@CyberAP For ref you can write like this:

const arr = ref([])

watch(
  arr,
  () => {
    console.log('Array: ', arr) // it will log after ref value changed or array changed.
  },
  { deep: true }
)

[CyberAP]

But what if I don't need deep reactivity here? (An array of nested arrays\objects?)

[unbyte]

@CyberAP could you please check again? if you try old version on online js runner such as codepen, they rewrite console.log so keys of array are depended internally.

that means it's not related to this PR but reactivity of vue 3

[yangmingshan]

But what if I don't need deep reactivity here? (An array of nested arrays\objects?)

I suggest to open a new issue, but it's not related to this PR.

[CyberAP]

@CyberAP could you please check again? if you try old version on online js runner such as codepen, they rewrite console.log so keys of array are depended internally.

that means it's not related to this PR but reactivity of vue 3

For the option breaking change I've made a separate issue: #2175
But the behaviour of watchEffect and watch Composition API methods needs the same treatment as well.

[unbyte]

so it's really unexpected for me that many are so eager to give reaction even before knowing the situation :)

[CyberAP]

@CyberAP could you please check again? if you try old version on online js runner such as codepen, they rewrite console.log so keys of array are depended internally.

that means it's not related to this PR but reactivity of vue 3

Not sure I follow, I've tested this locally in a plain html file and it doesn't work either. I don't understand how patching console.log is related here (you can put any effect the depends on array there).

[unbyte]

@CyberAP I've mistakenly thought you were testing the old version of vue 3 with platforms like codepen and latest version locally, so I said that. Anyway, what I mean is that it has nothing to do with this PR. Furthermore, I think this inconsistency is caused by the new reactivity system.

[CyberAP]

This is directly related to this PR. I would like to be able to have a reactive effect from pushing to a reactive array the same way it worked in Vue 2, this PR disables that. Deep watching an array, replacing the whole array or subscribing to an array length are not the same in terms of developer experience as simply array.push() to get the desired effect.
The fact that reactivity system is broken in other places is only partially related to this (and has been described in a separate issue), but the main point stays the same.

[unbyte]

so how do you like to solve such an infinite recursion

and this PR ONLY disabled not triggering but tracking keys like length inside methods like .push

[unbyte]

and also I've already tested this on versions from beta to one piece, and they all failed as I've alrady said

[edison1105]

and also I've already tested this on versions from beta to one piece, and they all failed as I've alrady said

你俩都比较轴，不是提了新的PR了嘛，尤大肯定会处理的。耐心等待。哈哈😊

[unbyte]

and also I've already tested this on versions from beta to one piece, and they all failed as I've alrady said

你俩都比较轴，不是提了新的PR了嘛，尤大肯定会处理的。耐心等待。

哈哈，确实有点杠上了，不过你看一下新的issue里的链接，尤大说是有意的
其实我想表达的只有“这不是我的锅” 😁

[edison1105]

and also I've already tested this on versions from beta to one piece, and they all failed as I've alrady said

你俩都比较轴，不是提了新的PR了嘛，尤大肯定会处理的。耐心等待。

哈哈，确实有点杠上了，不过你看一下新的issue里的链接，尤大说是有意的
其实我想表达的只有“这不是我的锅” 😁

哈哈 😄 方便留个微信吗，学习中有不懂的互相交流。

[yangmingshan]

@CyberAP You got it wrong, like @unbyte said this PR is only about not tracking length field when you access push/pop/shift/unshift/splice methods inside watchEffect or watch.

For now, watchEffect will only track fields that you accessed inside it, I think it designed for simple use case. As for your use case try watch instead.

const arr = ref([])

// if you only care about length
watch(
  () => arr.value.length,
  () => {
    console.log('Array: ', arr)
  }
)

[unbyte]

@edison1105 📧

[edison1105]

@edison1105 📧

already mail to u.

[CyberAP]

@yangmingshan the point stays the same. If you're pushing to a reactive array why this should be discarded? The whole point of mutating reactive value is to trigger reactivity. How's pushing to an array within an effect is different from pushing to an array outside of effect?

[yangmingshan]

It's not about pushing itself, pushing will works as it should be, push a new item to an array will still trigger effect run no matter inside effect or not. It's about unexpected tracking.

const arr = reactive([])

watchEffect(() => {
  arr.push(1) // this effect will run whenever the length of arr changes (before this PR), it shouldn't.
})

[CyberAP]

Why else it would have to trigger then? The code itself is recursive: an effect that mutates its own dependency. It is recursive in Vue 2 and I don't see any reason why it shouldn't be that way in Vue 3.

[Gkxie]

Why else it would have to trigger then? The code itself is recursive: an effect that mutates its own dependency. It is recursive in Vue 2 and I don't see any reason why it shouldn't be that way in Vue 3.

example 1

const arr = reactive([])
watchEffect(()=>{
  arr.push(1)
})
watchEffect(()=>{
  arr.push(2)
})

You know example 1 just want a push which means Invisibly side effect, that user code don't wish.

example 2

const arr = reactive([])
watchEffect(()=>{
  arr.length++
})
watchEffect(()=>{
  arr.length++
})

With example 2, the features of watchEffect is tracking obviousness dependency.

Keywords: obvious dependency.

[hyf0]

The situation looks similar to #3653. I'm also not convinced that it should be fixed.

I don't think this is a common use case. I think this PR should be revert and pending for not getting enough feedback of real world use case. I'm deeply worried that this PR can't be reverted for causing breaking change in the future. @yyx990803

[unbyte]

@iheyunfei Without this patch, you can't even invoke any statement that executes Array.push in the render function, including (el) =>arr.push(el), because it will trigger recursively rendering.