Skip to content

chore(reactivity): update toRef return value type #8464

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

Closed
wants to merge 2 commits into from

Conversation

Alfred-Skyblue
Copy link
Member

@Alfred-Skyblue Alfred-Skyblue commented May 31, 2023

toRef(()=>({a:1)) should return DeepReadonly, otherwise when the function returns an object or an array, modifying the object property ts will not report an error, but the modification will not be successful.

Expected:

const obj = toRef(()=>({a:1)) 
obj.value.a = 2 // Cannot assign to 'a' because it is a read-only property

console.log(obj.value.a) // 1

Actual:

const obj = toRef(()=>({a:1)) 
obj.value.a = 2 // success

console.log(obj.value.a) // 1

@Alfred-Skyblue Alfred-Skyblue changed the title test(reactivity): update toRef return value type chore(reactivity): update toRef return value type May 31, 2023
@sxzz sxzz added the ready to merge The PR is ready to be merged. label Aug 13, 2023
@skirtles-code
Copy link
Contributor

Is it safe to assume that the value returned from toRef() is deep readonly? If the value that's returned is an external reactive object (which seems quite a likely scenario in practice) then there should be no problem mutating a nested property:

const a = reactive({ b: { c: { d: 7 } } })

const c = toRef(() => a.b.c)

c.value.d = 6

In this scenario, I'd expect c to behave just like a computed(), allowing the assignment of c.value.d.

Perhaps I've misunderstood the proposed change, but it looks like it would prevent this kind of assignment.

In the original example given in the PR, the object in the value isn't readonly, it's just that a new object is returned each time the value is accessed. I'm not sure whether toRef() is intended to be used that way in practice. Wouldn't using either ref() or computed() be a more 'correct' way to handle a use case like that? Returning a new object each time seems likely to cause various other problems, e.g. obj.value === obj.value will be false. I think the callback passed to toRef() is expected to be 'stable' under repeat invocations.

@LinusBorg
Copy link
Member

I agree with @skirtles-code - this usage, while technically possible, doesn't really make sense in the context of what toRef is usually used for: getting a reactive reference to another stable value. And in that common usecase, mutating the stable result is a totally fine thing to do.

In my opinion this PR would break a lot of things and should not be merged.

@LinusBorg LinusBorg added need test The PR has missing test cases. need discussion and removed ready to merge The PR is ready to be merged. need test The PR has missing test cases. labels Oct 20, 2023
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 87.1 kB 33.1 kB 29.9 kB
vue.global.prod.js 133 kB 50 kB 44.8 kB

Usages

Name Size Gzip Brotli
createApp 48.4 kB 19 kB 17.4 kB
createSSRApp 51.6 kB 20.3 kB 18.5 kB
defineCustomElement 50.7 kB 19.8 kB 18.1 kB
overall 61.7 kB 23.9 kB 21.7 kB

Copy link

codspeed-hq bot commented Dec 20, 2023

CodSpeed Performance Report

Merging #8464 will not alter performance

Comparing Alfred-Skyblue:type-ref (bb2adc5) with main (9fa8241)

Summary

✅ 53 untouched benchmarks

@yyx990803 yyx990803 closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

5 participants