refactor: remove toRaw(ref) in triggerRefValue

[chenfan0]

It is not necessary to do toRaw(ref) for a RefBase<any> type

[github-actions]

Size Report

Bundles

File	Size	Gzip	Brotli
runtime-dom.global.prod.js	85.8 kB (-8 B)	32.6 kB	29.5 kB (+35 B)
vue.global.prod.js	132 kB (-8 B)	49.3 kB	44.3 kB (+33 B)

Usages

Name	Size	Gzip	Brotli
createApp	47.9 kB (-8 B)	18.8 kB (-2 B)	17.2 kB (-2 B)
createSSRApp	50.6 kB (-8 B)	19.9 kB (-1 B)	18.2 kB (+45 B)
defineCustomElement	50.2 kB (-8 B)	19.6 kB (-2 B)	17.9 kB (-3 B)
overall	61.2 kB (-8 B)	23.7 kB (-1 B)	21.5 kB (-47 B)

[sxzz]

/ecosystem-ci run

[vue-bot]

📝 Ran ecosystem CI: Open

suite	result
nuxt	✅ success
pinia	✅ success
quasar	❌ failure
router	✅ success
test-utils	✅ success
vant	✅ success
vite-plugin-vue	✅ success
vitepress	✅ success
vue-i18n	❌ failure
vue-macros	❌ failure
vuetify	✅ success
vueuse	✅ success
vue-simple-compiler	✅ success

[sxzz]

CI has failed. Could you please check toRaw is really necessary?

[chenfan0]

CI has failed. Could you please check toRaw is really necessary?

I'm not sure why ci fails, but judging from the vue source code, it is indeed unnecessary to perform toRaw on a RefBase<any> type.

[haoqunjiang]

Sorry, I messed up the ecosystem-ci comment 🤦‍♂️
Let me try again.

[haoqunjiang]

/ecosystem-ci run

[vue-bot]

📝 Ran ecosystem CI: Open

suite	result	latest scheduled
nuxt	✅ success	✅ success
pinia	✅ success	✅ success
quasar	✅ success	❌ failure
router	✅ success	✅ success
test-utils	✅ success	✅ success
vant	✅ success	✅ success
vite-plugin-vue	✅ success	✅ success
vitepress	✅ success	✅ success
vue-i18n	✅ success	✅ success
vue-macros	❌ failure	❌ failure
vuetify	✅ success	✅ success
vueuse	✅ success	✅ success
vue-simple-compiler	✅ success	✅ success

[skirtles-code]

I believe the toRaw is there to remove any proxies around the ref. Specifically, line 42 and 56 use toRaw to ensure the ref isn't wrapped in a readonly proxy.

While I think this makes sense in the case of trackRefValue, it is legitimate to wonder whether this is necessary for triggerRefValue. Can a ref be triggered via a readonly proxy?

The only way I could find to do this was by explicitly calling triggerRef on the wrapper.

A follow up question is: what harm does it do if we don't remove the wrapper?

If we don't unwrap the ref, then the values of target and dep passed to triggerEffects will still be wrapped in a proxy. The target is probably not important, as that is debug info, but the dep may be important.

Inside triggerEffects, the dep wrapper will carry over to triggerEffect. As a result, when triggerEffect attempts to check whether effect !== activeEffect, the equality check will fail because effect is wrapped in a proxy.

With that in mind, I created the following examples in the Playground:

• Current main branch
• This PR

With the current main branch, the watchEffect will not trigger itself recursively. But with the change in this PR it will, because the equality check I mentioned earlier is now failing to prevent the recursion.

Perhaps someone else can think of a simpler (less contrived) example, but I guess my point is just that this change is not simply a refactor, it does have an observable impact on how Vue behaves in edge cases.

[skirtles-code]

There have been significant changes to the reactivity system since this PR was opened, but the ref = toRaw(ref) line is still present, so I've just tested this change against the latest main branch.

With the new reactivity system, code like this will fail if we remove the ref = toRaw(ref) line:

import { readonly, shallowRef, triggerRef, watchEffect } from 'vue'

const msg = readonly(shallowRef(''))

watchEffect(() => {
  // ...
  triggerRef(msg)
})

The readonly proxy carries over to the dep, then to the effect, and we end up with warnings because _shouldSchedule and _dirtyLevel can't be set via the proxy.

Quite why somebody would want to trigger a ref via a readonly proxy I don't know, but it works currently and I don't think we should break it.

It would be nice to have a test for this edge case, but I think this change should be closed.