Skip to content

Conversation

edison1105
Copy link
Member

@edison1105 edison1105 commented Dec 26, 2023

simply demo
make sure switch to version 3.4 DEV mode.(PROD mode causes the browser to crash)

@johnsoncodehk
Copy link
Member

I'm skeptical that this use case should be handled, I don't think computed is designed to be used for recursive calculations, and c = computed(() => c.value...) should indeed be infinitely recursive in my opinion.

Even if the special case handles self-referencing, it still doesn't solve the more obscure similar use cases, e.g.:

const c = computed(() => d.value)
const d = computed(() => c.value)

I think we should just ask the user not to do this.

@johnsoncodehk
Copy link
Member

johnsoncodehk commented Dec 26, 2023

I prefer to throw an error for recursive calculations. To catch the above mentioned cases where c and d are accessing each other, or even more complex cases, effect._runnings should be checked instead of activeEffect.

  get value() {
    // the computed ref may get wrapped by other proxies e.g. readonly() #3376
    const self = toRaw(this)
+    if (self.effect._runnings) {
+      throw new Error(`recursive calculations`)
+    }
    if (!self._cacheable || self.effect.dirty) {
      if (hasChanged(self._value, (self._value = self.effect.run()!))) {
        triggerRefValue(self, DirtyLevels.ComputedValueDirty)
      }
    }
    trackRefValue(self)
    return self._value
  }

@yyx990803
Copy link
Member

I agree this isn't a realistic use case and the user should never do this. An error is more appropriate.

@edison1105
Copy link
Member Author

@johnsoncodehk done~

@edison1105 edison1105 changed the title fix(reactivity): avoid track computed effect during self access fix(reactivity): throw an error during computed self-referencing Dec 26, 2023
@yyx990803 yyx990803 added the 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. label Dec 26, 2023
@skirtles-code
Copy link
Contributor

Should this PR be updated to point at the main branch?

@edison1105
Copy link
Member Author

edison1105 commented Jan 13, 2024

the problem is fixed via https://github.com/vuejs/core/pull/10101/files
Not sure if still need to keep this PR 🤔

@edison1105 edison1105 closed this Jan 13, 2024
@edison1105 edison1105 reopened this Jan 13, 2024
@yyx990803
Copy link
Member

Closing as stale

@yyx990803 yyx990803 closed this Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants