-
Notifications
You must be signed in to change notification settings - Fork 33
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: Resolve issues in environments with frozen Symbol #48
Conversation
- Also adds a test for frozen Symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
// META: It's a resource locator! | ||
result = Symbol.for('https://github.com/benlesh/symbol-observable'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Very RDF
var forLookup = {}; | ||
|
||
FakeSymbol.for = function (id) { | ||
if (!forLookup[id]) { | ||
forLookup[id] = FakeSymbol(id); | ||
} | ||
return forLookup[id]; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems more complex that necessary. To emulate Symbol.for
, just avoid a counting prefix. Return the same string each time with the same fixed prefix. No state needed.
I will release this as a full major version change, as it is possible that there are esoteric run times that have I'm not certain that even exist though, so I'm not going to fix it in this release, as they can stick with a previous release and they would be fine. I will be sure to try to note that in the changelog. I will merge this and release tomorrow morning. |
Thank you for this patch.
According to caniuse.com, Symbol and Symbol.for have practically the same support. The only difference I see is in Chrome 38-39 (released in 2014) and Opera 25-26 (released in 2014). |
Related #47