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(render): Prevent error when displaying symbol keys on a map #9731

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

pikax
Copy link
Member

@pikax pikax commented Dec 1, 2023

closes #9727

playground

Copy link

github-actions bot commented Dec 1, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 86.8 kB (+61 B) 33 kB (+31 B) 29.8 kB (+16 B)
vue.global.prod.js 133 kB (+61 B) 49.8 kB (+30 B) 44.7 kB (+52 B)

Usages

Name Size Gzip Brotli
createApp 48.2 kB 19 kB 17.4 kB
createSSRApp 51.5 kB 20.3 kB 18.5 kB
defineCustomElement 50.6 kB 19.7 kB 18 kB
overall 61.6 kB 23.8 kB 21.7 kB

@jairoblatt
Copy link

@pikax I think this is not the best approach, since symbols accepts a description, replacing it with the map index may cause some misunderstandings.

Maybe this could be better:

entries[`${isSymbol(key) ? key.description ? `Symbol(${key.description})` /* or key.toString() */ : `Symbol(${i})` :  key} =>`] = val

Or just

 `Symbol(${key.description || i})`

@pikax pikax self-assigned this Dec 2, 2023
@pikax pikax removed their assignment Dec 2, 2023
@pikax
Copy link
Member Author

pikax commented Dec 2, 2023

@jairoblatt you right, thank you :)

Maybe this could be better:

entries[`${isSymbol(key) ? key.description ? `Symbol(${key.description})` /* or key.toString() */ : `Symbol(${i})` :  key} =>`] = val

Or just

 `Symbol(${key.description || i})`

I think the later is better because is shorter, String(key) should also work, since this is just a very edge case, I want to minimise the code size if possible.


The reasoning behind adding Symbol(i) is to render all the symbols, since by just using Symbol().toString() will always generate the same symbol, and since a map can possibly render multiple symbols, it would render just the last one, since they would share the same key.

I also thought in having the description changed for DEV (because of the extra 31 bytes added), but I don't think is a good usage, since this might be useful for production debugging, and having this discrepancy between DEV and PROD is not worth the saved 31 bytes IMO

@yyx990803 yyx990803 merged commit 364821d into main Dec 7, 2023
15 checks passed
@yyx990803 yyx990803 deleted the pikax/fix_render_Symbol_on_map branch December 7, 2023 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError can't convert symbol to string - Bug or limitation?
3 participants