-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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): toRef edge cases for ref unwrapping #12420
base: main
Are you sure you want to change the base?
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
with the following steps: 1. switch to main branch
2. nr prebench-compare
3. nr bench ref
4. switch to this PR
5. nr prebench-compare
6. nr bench-compare ref envinfo: System:
OS: macOS 14.5
CPU: (8) arm64 Apple M1
Memory: 127.44 MB / 16.00 GB
Shell: 5.9 - /bin/zsh
Binaries:
Node: 22.7.0 - /usr/local/bin/node |
@edison1105 Would you be able to try that performance benchmark again? I tried running it locally, but it seems to get stuck running I don't think my changes should impact those benchmarks. The benchmark you mentioned seems to be testing I would expect some performance impact on |
I tested it again. This time, I repeated step 6 above three times, and the results showed that performance was not affected. Below are the results from the third execution. The above test results were obtained from the first time run ✓ packages/reactivity/__benchmarks__/ref.bench.ts (4) 16730ms
✓ ref (4) 16728ms
name hz min max mean p75 p99 p995 p999 rme samples
· create ref 21,076,099.66 0.0000 0.2064 0.0000 0.0000 0.0001 0.0001 0.0002 ±0.23% 10538050 [1.00x] ⇓
create ref 21,152,011.37 0.0000 0.5231 0.0000 0.0000 0.0001 0.0001 0.0002 ±0.38% 10576006 (baseline)
· write ref 20,557,804.48 0.0000 0.4285 0.0000 0.0000 0.0001 0.0001 0.0001 ±0.51% 10278903 [1.02x] ⇑
write ref 20,076,703.63 0.0000 0.5026 0.0000 0.0000 0.0001 0.0001 0.0002 ±0.52% 10038353 (baseline)
· read ref 24,080,925.98 0.0000 0.7341 0.0000 0.0000 0.0000 0.0001 0.0002 ±1.10% 12040464 [0.99x] ⇓
read ref 24,253,846.74 0.0000 1.1237 0.0000 0.0000 0.0000 0.0001 0.0001 ±1.02% 12126924 (baseline)
· write/read ref 19,876,380.25 0.0000 0.5281 0.0001 0.0000 0.0001 0.0001 0.0002 ±0.64% 9938191 [1.03x] ⇑
write/read ref 19,240,027.69 0.0000 2.3183 0.0001 0.0000 0.0001 0.0001 0.0002 ±1.21% 9620095 (baseline) |
This PR aims to address several related edge cases.
Case 1
The original motivation came from vuejs/pinia#2812.
Long story short, consider the following code:
The call to
toRef
currently triggers the logging, even if the returned ref is never used:While attempting to fix this, I kept running into other edge cases, mostly involving
toRef
andshallowReactive
/shallowReadonly
. I've tried to fix all of those edge cases too.Case 2
Consider the following:
This should result in
t.value
being1
, but currently it ends up as0
.Case 3
Consider this example:
t.value
should be7
, respecting the default value. But currently it isundefined
.Case 4
This is somewhat separate, but it impacted the tests I wrote for array handling with
toRef
.Consider the following:
Arrays don't unwrap refs used as elements, but they do unwrap arrays added using custom properties such as
foo
. Essentially, custom properties on reactive arrays behave just like properties of normal reactive objects. Apart from in the case outlined above, where the ref is replaced rather than updated byarr.foo = 2
. I've changed this inbaseHandlers.ts
, so that updating a custom property on an array behaves the same as for an object:Notes on the changes
The key change is to move the logic for handling nested refs from
propertyToRef
to insideObjectRefImpl
.Detecting whether a source will automatically unwrap refs is a bit tricky. It isn't sufficient just to check for
isShallow
, as you could haveshallowReadonly
wrapped aroundreactive
. Instead I've recursively checked each wrapper proxy to check whether any level would unwrap.As noted earlier, arrays also pose a specific challenge, as they don't unwrap elements even inside
reactive
orreadonly
.There is one existing test case that no longer passes. Specifically, this one:
This is expecting the exact same ref to be returned by
toRef
. But returning the same ref is the underlying cause of most of the edge cases outlined above. From what I can tell, the original motivation for that test case was just to ensure that nested refs are unwrapped, not that they need to be===
. Returning an equivalent ref should be sufficient.I do wonder whether there's an easier way to implement all of this, but currently I'm not seeing it.