Skip to content

refactor: use symbol for private properties #8681

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

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

Alfred-Skyblue
Copy link
Member

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

I have seen many places where HTMLElement's properties are extended, such as the addition of the _assign property to the Element in vModel. Should we use Symbol properties for such extensions? This would prevent users from accessing and tampering with these properties.

@sxzz sxzz requested a review from yyx990803 August 13, 2023 17:50
@sxzz sxzz changed the title chore: extend HTMLElement with Symbol as key refactor: extend HTMLElement with Symbol as key Aug 13, 2023
@Alfred-Skyblue Alfred-Skyblue requested a review from sxzz August 14, 2023 01:58
@sxzz sxzz added the ready for review This PR requires more reviews label Aug 14, 2023
@sxzz
Copy link
Member

sxzz commented Aug 14, 2023

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Aug 14, 2023

📝 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 ✅ success
vue-macros ❌ failure
vuetify ✅ success
vueuse ✅ success
vue-simple-compiler ✅ success

@Alfred-Skyblue Alfred-Skyblue force-pushed the chore-useSymbol branch 2 times, most recently from 577032e to 703bc1d Compare August 14, 2023 10:48
@Alfred-Skyblue Alfred-Skyblue requested a review from sxzz August 14, 2023 10:52
@sxzz
Copy link
Member

sxzz commented Aug 14, 2023

Thank you for your excellent work! However, I am concerned that using symbols as keys may incur greater performance overhead compared to strings. Can you attempt to test whether it will have a significant impact on performance? If so, we need to consider if this PR is truly worth it. If there is not much performance degradation, we believe your PR is valuable!

@Alfred-Skyblue
Copy link
Member Author

14271692021148_ pic

Taking the example of running "set" 10, 100, and 1000 times, as the quantity increases, the performance difference between symbols and strings will become smaller. Of course, if we consider ten runs, symbols take 0.078ms, while strings take 0.015ms.

@sxzz
Copy link
Member

sxzz commented Aug 14, 2023

https://jsben.ch/9mNDi

I have tested using both string keys and symbol keys separately. The results show that the performance is almost the same, with a fluctuation between 98% and 102%. Sometimes, symbols even perform slightly faster than strings.

In conclusion, I believe that your PR has a minimal impact on performance.

@sxzz sxzz added ready to merge The PR is ready to be merged. and removed ready for review This PR requires more reviews labels Aug 15, 2023
@sxzz sxzz force-pushed the chore-useSymbol branch from 703bc1d to dbda208 Compare August 21, 2023 07:12
@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.8 kB (+19 B) 32.7 kB (+69 B) 29.5 kB (+103 B)
vue.global.prod.js 132 kB (+19 B) 49.4 kB (+58 B) 44.3 kB (+20 B)

Usages

Name Size Gzip Brotli
createApp 47.8 kB (+60 B) 18.9 kB (+11 B) 17.2 kB (+27 B)
createSSRApp 50.6 kB (+60 B) 20 kB (+21 B) 18.2 kB (+17 B)
defineCustomElement 50.2 kB (+60 B) 19.6 kB (+14 B) 17.9 kB (+11 B)
overall 61.2 kB (+39 B) 23.7 kB (+13 B) 21.6 kB (+7 B)

@sxzz sxzz changed the title refactor: extend HTMLElement with Symbol as key refactor: use symbol for private properties Aug 22, 2023
@sxzz sxzz merged commit 2ffe3d5 into vuejs:main Aug 22, 2023
@Alfred-Skyblue Alfred-Skyblue deleted the chore-useSymbol branch August 24, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants