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(reactivity): ref as a reactive parameter should return (#6358) #6359

Closed
wants to merge 6 commits into from

Conversation

hubvue
Copy link

@hubvue hubvue commented Jul 26, 2022

fix: #6358

It need to wait for #11696 to be fixed

@skirtles-code
Copy link
Contributor

The title of this PR is currently a bit misleading. It refers to an earlier iteration of the code, but the code has changed significantly since then.


Some notes for reviewers...

There's another PR, #6376, that attempts to fix the same problem. They take very different approaches, so I've written a brief comparison here.

The original problem occurs with this code:

import { reactive, ref, effect } from 'vue'

const a = reactive(ref(1))

effect(() => {
  console.log(a.value)
})

a.value++

The effect ends up with 3 dependencies, so when the value changes to 2, it will trigger the effect 3 times. The dependencies are:

  1. The value property of the ref.
  2. The value property of the reactive proxy.
  3. The _value property of the reactive proxy.

The approach in #6376 is to remove the proxy before accessing _value. That avoids dependency 3 in the list above, so the effect is only triggered twice.

The approach in this PR is a bit different. It skips tracking in the proxy if the underlying value is a ref. That avoids dependencies 2 and 3 from the list above, so the effect is only triggered once.

This does also mean that the proxy will no longer track any other properties of the ref, not just value and _value. Why would a ref have other properties? I don't know, but I also don't know why a ref would be wrapped with reactive in the first place, so I find it difficult to speculate about what the expected behaviour would be.

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 99.1 kB (+15 B) 37.5 kB (+12 B) 33.8 kB (-29 B)
vue.global.prod.js 157 kB (+15 B) 57.3 kB (+9 B) 51 kB (+6 B)

Usages

Name Size Gzip Brotli
createApp 54.5 kB (+15 B) 21.1 kB (+8 B) 19.3 kB (+12 B)
createSSRApp 58.5 kB (+15 B) 22.8 kB (+8 B) 20.8 kB (+1 B)
defineCustomElement 59.1 kB (+15 B) 22.6 kB (+8 B) 20.6 kB (+16 B)
overall 68.1 kB (+15 B) 26.2 kB (+18 B) 23.9 kB (+7 B)

Copy link

pkg-pr-new bot commented Aug 23, 2024

commit: a2ced04

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@6359

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@6359

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@6359

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@6359

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@6359

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@6359

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@6359

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@6359

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@6359

vue

pnpm add https://pkg.pr.new/vue@6359

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@6359

Open in Stackblitz

@edison1105 edison1105 added the 🛑 on hold This PR can't be merged until other work is finished label Aug 28, 2024
@yyx990803
Copy link
Member

This is no longer needed in 3.5 after 313e4bf

@yyx990803 yyx990803 closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛑 on hold This PR can't be merged until other work is finished scope: reactivity
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

ref is called as a reactive parameter and effectFn is triggered multiple times when updated
5 participants