Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Array.splice() to watch array #2116

Open
zmtlwzy opened this issue Sep 14, 2020 · 20 comments
Open

Use Array.splice() to watch array #2116

zmtlwzy opened this issue Sep 14, 2020 · 20 comments
Labels
has workaround A workaround has been found to avoid the problem scope: reactivity

Comments

@zmtlwzy
Copy link

zmtlwzy commented Sep 14, 2020

Version

3.0.0-rc.10

Reproduction link

https://codepen.io/zmtlwzy/pen/oNxypGz

Steps to reproduce

Use Array.splice or sort to watch array in vue2 (can use),
but in vue3 cannot use.

vue2: https://codepen.io/zmtlwzy/pen/ExKRoNY
vue3: https://codepen.io/zmtlwzy/pen/oNxypGz

What is expected?

can watch in vue3

What is actually happening?

No effect

@posva
Copy link
Member

posva commented Sep 14, 2020

You need to add a deep: true to the watcher options. I couldn't find this breaking change but I'm also not sure if this was intended. If it was, it should be added to the migration guide

@zmtlwzy
Copy link
Author

zmtlwzy commented Sep 14, 2020

I find this api from : https://v3.vuejs.org/api/instance-methods.html#watch
It's works better than vue2
thanks !

@yyx990803
Copy link
Member

This is somewhat intended. Technically, watch: 'key' should only trigger when the binding itself has been mutated - Vue 2's behavior is overly-eager because due to ES5 restrictions we have to track more than we should when Arrays are encountered. Vue 3's reactivity tracking is more precise thanks to Proxies, so in this case it won't watch the Array's own mutations unless deep: true is provided.

I'm not sure if we should make v3's behavior strictly the same in this case, as it would entail the same tracking behavior when you do watch(() => object.nestedArray, ...). (Maybe we should)

@yyx990803 yyx990803 added the has workaround A workaround has been found to avoid the problem label Sep 16, 2020
@LinusBorg
Copy link
Member

@yyx990803 I think we should keep the Vue 3 behavior as is. The Vue 2 behavior was out fo necessity. IU think with the old caveat out of the way, objects and arrays now behave the same - and both need a deep: true to have changes to their properties watched.

Artificially reverting to the Vue 2 behavior for arrays seems counterproductive in terms of a better, more unified API, even if it's a breaking change, technically. (probably not in the migration docs yet).

The only thing giving me doubts is the IE11 build, where we the old behavior would necessarily surface again ...

@CyberAP
Copy link
Contributor

CyberAP commented Oct 28, 2020

@LinusBorg but how are end users are going to benefit from the unified behaviour for both objects and arrays? I couldn't think of a case where we explicitly don't want to watch for array mutations, but I certainly can imagine cases where we don't want to watch for object mutations. Objects and arrays are used differently, so it might make sense to treat them differently as well to create a better developer experience overall, even though the behaviour might not be consistent.

Let me provide you with an example of what I mean:

const object = ref({ foo: 'bar' })
const array = ref(['foo'])

Depending on a watched source I'd expect a different behaviour:

  • For watch(object, callback) I would expect the callback to trigger only if the ref value changes. For example when we empty the value with object.value = null or replace it with another value. If I need to be notified of property mutations I'd add a deep: true as well. This is convenient because if the object is monomorphic I won't get extra triggers from object properties changes (which are far more frequent than object mutations). And at the same time I can opt-out of that behaviour with deep: true or just watch a property directly.
    Vue 2 has emphasized monomorphic objects and required Vue.set\Vue.delete for everything else. And that limitation greatly contributed to the approaches you have to consider to describe your application state. So building up on that idea in my opinion is a great way to go and the way it works right now with objects is perfectly fine.

  • For watch(array, callback) I would expect the callback to trigger on every array mutation and ref value change. Because arrays on the other hand are most always never monomorphic and can contain an arbitrary number of properties. We don't usually fill objects with lots of new properties, but with arrays it's one of the most frequent things that we do. And in most cases that's exactly what we would like to watch for, not just the value emptiness. That's why it's quite hard to comprehend why there's need to use deep: true when we would like to watch for array mutations, because it seems that's exactly where the reactivity system could shine at most.

Lastly, I'd like to point out that render context has the right behaviour regarding arrays. When array is used in a v-for cycle it will trigger render on array mutations with no extra work required to do and when I am working with reactivity I would like this behaviour to be present in Composition and Options API as well.

@LinusBorg
Copy link
Member

I get your argument about differences in main use cases. At he same time, I also think when designing an API, providing consistent behaviors and and avoiding surprises should be an important factor as well.

If feel like watching a ref should only trigger when the ref's value changes - not matter what kind the value of the ref might be.

With your proposal, the default behavior would be different for arrays only, and people might not even be aware that their watchers run unnecessarily on every mutation of an array, when they might want to run it on each value reassignment only.

Sure, once they are aware of this "weird" difference in behavior they can short-circuit the watcher by doing something like this:

if (newArray === oldArray) return

...but that assumes they are even aware that it happens.

Adding to that, in my personal experience at least, wrapping an array in a ref is usually done in situations where you do regularly replace the whole array, i.e. from API calls & refreshs, whereas I find myself using a reactive(array) often for stuff that gets mutated a lot.

Lastly, you can get the behavior you want (watching for array length changes, basically) with the current implementation at the small cost of a few more characters:

const array = ref(['foo'])

watch(
  () => [...array.value],
  (newArray, oldArray) => {
    // do something
  }
)

Here, you even get the benefit of having the previous array state to compare! (fiddle)

So in summary:

  • watching a ref should do the same for objects and array in order to be consistent and unsurprising in its behavior
  • the common use case of watching an array for mutations can be achieved with relative ease and added benefits of true new/old comparison of the array's state.

@CyberAP
Copy link
Contributor

CyberAP commented Oct 29, 2020

Should the watch trigger if the reactive property is mutated then?

const state = reactive({ items: ['foo'] })

watch(() => state.items, callback)

state.items.push('bar') // should it trigger watcher?

@LinusBorg
Copy link
Member

LinusBorg commented Oct 29, 2020

Nope. For that, we have deep: true or, as mentioned, this:

watch(() => [...state.items], callback)

@CyberAP
Copy link
Contributor

CyberAP commented Oct 29, 2020

I guess in that case it should stop triggering watcher if I watch the state object only.

const state = reactive({ items: ["foo"] })

watch(state, callback)

state.items.push('bar') // will trigger watcher

But I still don't find it useful that it behaves exactly like objects, but maybe that's only me.

@LinusBorg
Copy link
Member

LinusBorg commented Oct 29, 2020

Hm. Yeah tbh I wasn't event aware of this. it's implicitly doing a deep watch - personally I don't think that's a good thing, deep watches can be expensive :-/

I now realize it was probably done so people don't wonder why nothing happens when they watch pass an object by itself, but ... not sure I like it.

@LinusBorg
Copy link
Member

LinusBorg commented Oct 29, 2020

anyway, the behaviour is consistent with objects and arrays, at least:

/* REACTIVE */
const obj = reactive({})
const array = reactive([])

watch(obj, cb) // implicit deep: true
watch(array, cb) // implicit deep: true

obj.newProp = 'foo' // => triggers cb
array.push('foo') // => triggers cb

/* REFS */

const refObj = ref({})
const refArray = ref([])

watch(refObj, cb)
watch(refArray, cb)

refObj.value.newProp = 'foo' // => does *not* trigger cb
refArray.value.push('foo') // => does *not* trigger cb


/* FUNCTION */
const obj = reactive({})
const array = reactive([])

watch(() => obj, cb) // => does *not* trigger cb at all
watch(() => array, cb) // => does *not* trigger cb at all

watch(() => ({...obj}), cb) // => does trigger cb on mutation, but not deep
watch(() => [...array], cb) // => does trigger cb on mutation, but not deep.

@LinusBorg
Copy link
Member

@yyx990803 I would propose to close this and leave the behaviour as is. WDYT?

@WesleyKapow
Copy link

[...array] approach is working for me. I'd suggest adding a note about this in the docs. It was definitely not obvious that watching an array does not watch adds/removes.

@Shinigami92
Copy link
Contributor

[...array] approach is working for me. I'd suggest adding a note about this in the docs. It was definitely not obvious that watching an array does not watch adds/removes.

Yeah, took me around 20min to find out how I can workaround that
A note here https://v3.vuejs.org/guide/migration/watch.html would be nice 🙂

@scriptsman
Copy link

anyway, the behaviour is consistent with objects and arrays, at least:

/* REACTIVE */
const obj = reactive({})
const array = reactive([])

watch(obj, cb) // implicit deep: true
watch(array, cb) // implicit deep: true

obj.newProp = 'foo' // => triggers cb
array.push('foo') // => triggers cb

/* REFS */

const refObj = ref({})
const refArray = ref([])

watch(refObj, cb)
watch(refArray, cb)

refObj.value.newProp = 'foo' // => does *not* trigger cb
refArray.value.push('foo') // => does *not* trigger cb


/* FUNCTION */
const obj = reactive({})
const array = reactive([])

watch(() => obj, cb) // => does *not* trigger cb at all
watch(() => array, cb) // => does *not* trigger cb at all

watch(() => ({...obj}), cb) // => does trigger cb on mutation, but not deep
watch(() => [...array], cb) // => does trigger cb on mutation, but not deep.

If I add deep: true to trigger callback, I can't get the correct oldValue.

Check this
https://codepen.io/scriptsman/pen/gOmjXxy

@slavicd
Copy link

slavicd commented Apr 26, 2022

Well, this is disappointing. I lost a few hours because, contrary to what the docs say, and contrary to all the logic, array.push() is "reactive" in the sense that my views are changed, yet I can't watch it as I always could. This is broken behaviour.

@cmcnicholas
Copy link

Well, this is disappointing. I lost a few hours because, contrary to what the docs say, and contrary to all the logic, array.push() is "reactive" in the sense that my views are changed, yet I can't watch it as I always could. This is broken behaviour.

agreed, I have spent a few hours teasing this out now too, not great.

@WORMSS
Copy link

WORMSS commented Oct 31, 2022

This may be a problem between how the data within the array.
There are times when we want the watcher to be triggered when a new item is added to the array so we are now required to use { deep: true },
but we are holding arbitrary information on objects within that array, and DO NOT want to trigger the watcher when this background information has been changed. So in that case we required { deep: false }

@LinusBorg
Copy link
Member

So in that case we required { deep: false }

You are not required to do that. A custom getters allows you to have fine-grained control over any part of the process:

// track array length only
watch(() => myArray.length, () => ...)
// or have the getter iterate over the array, i.e. with .slice()
// this would detect any structural change like a .sort() or .reverse(), where length would not change
watch(() => [...myArray], () => ...)

Examples:

https://sfc.vuejs.org/#eNqNU02P0zAQ/SuDhZQspM4u3Kq2sOLCAW5IHOpK6ybTxItjW7bTahXlv2M7aQta0O4lntHMe/MyHwO5N4YeeyRLsnKVFcaDQ9+bDVOiM9p6GMDioQgfXnlxxAJO3FctjHCwuoMsYDOmmKq0ch4618A6AvLsK0qp4ae2sn6T3VwSnu6t5U8paSLMt4NYwt1YQHw/zO/HcRcwTKViOVMA+Q2sN2c8laga3xYpMNugZf0tmSlziDGAWFZLpFI3+YPw2EHLHewRFbwdJiRsrlD4BBmva6wzWEJmsdPHaI8PQQ3AyFR6n+vaUkpnbbtJlcJTcpOuZP1PVua87SvfWy6harlqMJQu/oG/SliV07DCmIIT/spI7jF4AKt9771W8LmSovq1ZuTcMtO7No+9/buH8D40/4aRBAYIP59YyonmBUpt8it06tbr0RaPaB3+wfBDN43EQJQCr2ba3u6oCCv1nfuWHqTWNk+m5arWXRjQO7i7vVb5knoMNaKJc/Co/PNSvZyzV1LAcXHQNtRL+yPUuYMXRoBhgBgMMsYwoYQrpZioypkrju0yKlKQ6cAWHTf00WkVTjAtRyiTAo6R5XldGAl3Fn1GWu+NW5alO1TxcB8d1bYpg0Vtr7zokKLrFnurTw5tIGZkXpuRjL8BW0xONQ==

@WORMSS
Copy link

WORMSS commented Oct 31, 2022

This seems REALLY bad for large arrays for memory churn. This wouldn't even stop the spread early if it detected the first item had changed, it would still keep spreading all the way to the 100,000th item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has workaround A workaround has been found to avoid the problem scope: reactivity
Projects
None yet
Development

No branches or pull requests