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): fix shallowReactive map "unwraps" the nested refs #8502

Closed
wants to merge 7 commits into from

Conversation

Alfred-Skyblue
Copy link
Member

@Alfred-Skyblue Alfred-Skyblue commented Jun 5, 2023

fixed #8501

I found inconsistent behavior with Map.set in #8501, this is a breaking change, please close this PR if this is intentional

@phonty29
Copy link

nice !

@pikax
Copy link
Member

pikax commented Oct 20, 2023

linking #8503 because it fix the same issue

Copy link

github-actions bot commented Nov 9, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 87.1 kB (+18 B) 33.1 kB (+7 B) 29.9 kB (-3 B)
vue.global.prod.js 133 kB (+18 B) 50 kB (+7 B) 44.8 kB (+65 B)

Usages

Name Size Gzip Brotli
createApp 48.4 kB (+18 B) 19 kB (+4 B) 17.4 kB (-50 B)
createSSRApp 51.6 kB (+18 B) 20.3 kB (+5 B) 18.5 kB (+1 B)
defineCustomElement 50.8 kB (+18 B) 19.8 kB (+5 B) 18.1 kB (-51 B)
overall 61.7 kB (+18 B) 23.9 kB (+4 B) 21.7 kB (+13 B)

Copy link

codspeed-hq bot commented Dec 20, 2023

CodSpeed Performance Report

Merging #8502 will degrade performances by 42.36%

Comparing Alfred-Skyblue:fix-shallowReactive (baaf820) with main (9fa8241)

Summary

❌ 3 regressions
✅ 50 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main Alfred-Skyblue:fix-shallowReactive Change
write reactive map, don't read computed (never invoked) 62.2 µs 70.1 µs -11.37%
write reactive map property 85.8 µs 100.8 µs -14.86%
write reactive map, don't read 1000 computeds (never invoked) 76.7 µs 133 µs -42.36%

Copy link
Contributor

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, the changes proposed here are incorrect. The test expectations should not need to change for reactive() collections.

I think the fix proposed in #8503 is closer to the desired behaviour.

packages/reactivity/src/collectionHandlers.ts Show resolved Hide resolved
@Alfred-Skyblue Alfred-Skyblue deleted the fix-shallowReactive branch August 8, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

Setting a reactive object on a shallowReactive map "unwraps" the nested refs in the reactive object
4 participants