[core] Guard against bad Symbol polyfills #17336
Conversation
The `typeof` check is great but, apparently, some `Symbol` polyfills do not include `for()`; a further check would be preferable.
Details of bundle changes.Comparing: f8076d6...ecc4179
|
|
@briandelancey Thanks for the pull request, I have updated it to replicate React codebase strategy: |
oliviertassinari
changed the title
oliviertassinari
added
core
| @@ -1,3 +1,3 @@ | |||
| const hasSymbol = typeof Symbol === 'function'; | |||
| const hasSymbol = typeof Symbol === 'function' && Symbol.for; | |||
|
|
|||
| export default (hasSymbol ? Symbol.for('mui.nested') : '__THEME_NESTED__'); | |||
There was a problem hiding this comment.
I'm just hijacking this to fill in my own gaps: What's the point of a global Symbol.for registry? If we fallback to strings in bad polyfills anyway why not just use them? Is this some micro optimization I'm missing?
There was a problem hiding this comment.
@eps1lon I have "almost" blindly copied the react approach. I think that the point is to make it internal, it's not something we want developers to hack around. Symbol.for should be supported in 99% of the developers' environment even if this number is lower in end users.
There was a problem hiding this comment.
Yeah no this is more about what Symbol.for is useful since it's anything but internal. The mdn docs even recommend prefixing it to avoid name collisions. What does Symbol.for('mui.nested') do that 'mui.nested' can't?
There was a problem hiding this comment.
What does Symbol.for('mui.nested') do that 'mui.nested' can't?
I would say, let's compare
var theme1 = {
[Symbol.for('mui.nested')]: true,
}
var theme2 = {
'__THEME_NESTED__': true,
}
console.log(theme1, theme2)Which variant makes you less interested in hacking with the value?
There was a problem hiding this comment.
@briandelancey what's your perspective on the issue?
There was a problem hiding this comment.
I was not proposing to deviate. When in doubt assume the author added it with intent. Just thought anybody know what technical benefits Symbol.for has.
There was a problem hiding this comment.
@eps1lon ok, sounds good to me. Is it ready to ship?
There was a problem hiding this comment.
Thanks for raising this point.
There was a problem hiding this comment.
@oliviertassinari I'm ok with https://stackoverflow.com/questions/51488519/what-advantage-of-using-symbol-forstring-instead-the-plain-string
Not being enumerate by the common Object.keys seams reasonable. And since the theme is often passed around (potentially stringified for hydrated UIs) having it not appear for JSON.stringify serialization is also nice. So it's better at emulating internal properties.
eps1lon
changed the title
|
@briandelancey Thanks 👍 |
|
@eps1lon @oliviertassinari - First off, thanks for getting this in. Secondly, any idea when the next release might be? We're going to production with a project very soon that really needs this fix... |
|
@briandelancey v4.4.1 should be released this weekend. |
The
typeofcheck for Symbol is great but, apparently, some Symbol polyfills do not includefor(); a further check would be preferable.