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

fix(baseHandlers): trigger will called two times when set the same proxy value #2904

Merged
merged 5 commits into from
Mar 29, 2021

Conversation

liaoliao666
Copy link
Contributor

@liaoliao666 liaoliao666 commented Dec 29, 2020

import { reactive, toRaw, watchEffect } from 'vue'

const a = reactive({
  a: 1,
})

const b = reactive({
  b: 1,
})

const d = reactive([a, b])

watchEffect(() => {
  console.log('d', d[0]) // Will consoled two times
})

d[0] = a

@posva
Copy link
Member

posva commented Dec 29, 2020

I would say watchEffect is different from watch which does observe the value: watchEffect tracks changes to d.

@liaoliao666
Copy link
Contributor Author

I would say watchEffect is different from watch which does observe the value: watchEffect tracks changes to d.

i get ur mean, but why does set the same value will trigger the effect. im still confused

@LinusBorg
Copy link
Member

LinusBorg commented Dec 29, 2020

@posva I think he has a point here, but not sure if the change actually is the right fix. The lack of a test to verify doesn't help here.

What's the issue?

hasChanged(value, oldValue) is meant to guard against triggering an effect if the new and old value being set are the same, and it should keep the watchEffect from running in this instance, as a === a.

But in the given example, the original array from which d was reactively constructed, contains a reactive proxy, and so now we end up effectively comparing the raw value to the proxy from the source array, because value has been set to the raw value before, but oldValue has not:

https://github.com/vuejs/vue-next/blob/e5a5a2cb8d9c38f5fca52a942361123ebd402fb7/packages/reactivity/src/baseHandlers.ts#L136-L138

I think this fix is valid but definitely needs a test case added to the suite and maybe some thinking about other edge cases.

@posva
Copy link
Member

posva commented Dec 29, 2020

It's true this could be an optimization and the change makes sense

@LinusBorg LinusBorg added the need test The PR has missing test cases. label Dec 29, 2020
@LinusBorg
Copy link
Member

@liaoliao666 can you add a test case?

@liaoliao666
Copy link
Contributor Author

@liaoliao666 can you add a test case?

okay

@LinusBorg LinusBorg added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: reactivity and removed need test The PR has missing test cases. labels Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: reactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants