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(shared): unwrap refs in toDisplayString #7306

Merged
merged 5 commits into from
Jun 23, 2024

Conversation

skirtles-code
Copy link
Contributor

@skirtles-code skirtles-code commented Dec 9, 2022

Closes #5578.


From the docs, https://vuejs.org/guide/essentials/reactivity-fundamentals.html#ref-unwrapping-in-templates:


image


This isn't quite true. The value of the ref will be passed through JSON.stringify(), even if the value is a primitive. For a number or a boolean this will just yield the expected string, but for a string value it leads to extra quotes and escaping:

https://sfc.vuejs.org/#eNo9j8FOw0AMRH/F+BKQ6OYeFSRufIQvbXAgUXa9Wjv0EO2/420qbh579Ga840fO4XdjHPCsY5mzgbJt+Z3SHLMUgx0KT1BhKhKhc2tHidIoSQ3kuvBo8AY7JYBJZGjm5+6T11WA8CZl/SJ86l4oVUrn/ohwuAvjmNeLsSuAfX/AglOg3s3/BnzFo8wpXnJYVJLXvUfS46CEw1Gi7bxk04Q/ZlmHvtdpbE8uGqR89z6FsiWbIwfWeLoWuSkXBxM2hIdXrH+TUWJK

This PR aims to fix that problem by making {{ object.foo }} and {{ object.foo.value }} equivalent for a ref object.foo.

Notes:

  • The performance impact should be negligible. The extra isRef check is only performed when the value is an object, so it won't occur in the most common cases.
  • toDisplayString now calls itself recursively. While that has the potential to be problematic, in practice I don't think this introduces any new problems, as replacer is doing the same thing.

@github-actions
Copy link

github-actions bot commented Oct 20, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 89.5 kB (+46 B) 34.1 kB (+36 B) 30.7 kB (-1 B)
vue.global.prod.js 147 kB (+45 B) 53.3 kB (+6 B) 47.6 kB (-21 B)

Usages

Name Size Gzip Brotli
createApp 49.8 kB 19.5 kB 17.8 kB
createSSRApp 53.2 kB 20.8 kB 19 kB
defineCustomElement 52.1 kB 20.3 kB 18.5 kB
overall 63.3 kB 24.4 kB 22.2 kB

# Conflicts:
#	packages/shared/src/toDisplayString.ts
Copy link

codspeed-hq bot commented Dec 13, 2023

CodSpeed Performance Report

Merging #7306 will not alter performance

Comparing skirtles-code:interpolation-ref-unwrap (4b4a4bb) with main (a6503e3)

Summary

✅ 53 untouched benchmarks

@skirtles-code
Copy link
Contributor Author

I was unaware of #5593 when I opened this. Here's a brief comparison:

  1. This PR adds several extra test cases. Both fixes for toDisplayString pass those extra test cases. I believe the fixes are functionally equivalent.
  2. Inside toDisplayString, #5593 handles the ref case first, whereas this PR handles it as late as possible. This likely has some performance implications, albeit only on a very small scale. The way I've implemented it here, the check for a ref will only happen if it has gone down the 'object' branch, so it shouldn't have any impact on any other branches. The approach in #5593 will always do the ref check. In my opinion, it isn't a common enough usage to justify doing that check first.
  3. I've extracted the ref check to a separate function, as it's used in two places. While this may have performance implications, I think it's worth keeping in mind that the function won't actually be called in the most common code paths.
  4. The two PRs use any and unknown types slightly differently. I believe the way I've done it here is slightly safer, as very little of the code is handling an any value.

@haoqunjiang haoqunjiang added 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready for review This PR requires more reviews labels May 29, 2024
@yyx990803 yyx990803 merged commit 0126cff into vuejs:main Jun 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready for review This PR requires more reviews
Projects
Development

Successfully merging this pull request may close these issues.

Missleading rendering: Ref or computed inside non-wrappable variables (objects, arrays) which returns a string
3 participants