Updated csp.CLOSED to Symbol.for('@@csp/CLOSED') with fallback to Object String #61
Conversation
|
👍 |
|
I don't think this does what you think it does. In Firefox: |
|
@jlongster that's just how the console displays it. |
|
Yeah, I suppose the one potential issue to this approach vs a plain object is it's potentially confusing in logging situations, but it DOES allow for a bunch of useful applications without interference with the main For reference, |
|
they'll |
|
Much better use |
|
But Symbols has smaller set of supported browsers, of course |
|
yeah, even symbols has problems though since it's too easy to create duplicate packages in npm. What some people are doing are just namespacing strings. That's what we're doing in transducers: cognitect-labs/transducers-js#20 This works when you have multiple versions of the library (the above Object trick wouldn't) and even can be used across multiple libraries (if there is another CSP lib). So I'd say just do with something like I'll really miss being able to do |
|
Incidentally, I'd originally written it with Symbols and a fallback to an empty object, but switched to this after discussing the general practice. Code looked something like this (but changed to using Symbol.for to add it to the runtime Symbol registry, which should solve the multiple implementation issue): EDIT: Updated to use @@csp/CLOSED instead of csp::CLOSED because.. why not? var CLOSED;
try {
CLOSED = Symbol.for('@@csp/CLOSED');
// check for a properly-behaving ES6 Symbol implementation
if (typeof CLOSED !== 'symbol') throw new Error('Symbols not supported.');
} catch(e) {
// could use Object('@@csp/CLOSED') here instead?
CLOSED = {};
}I went with object strings here on the basis that they accomplish nearly the same effect while being evaluated/compared faster, and wouldn't suffer from the unpredictability of the above (even if that unpredictability SHOULD be moot under most use-cases). The other downside of a Symbol is that if used as a key in a lookup object, it wouldn't appear in Object.keys(obj). @jlongster good point about other libs and/or other instances, though. However, folks could work around that when necessary by using soft equality. Still, it does feel a little ugly that way - which is a point in favor of @@csp/CLOSED. |
|
The problem with "sometimes Symbol and sometimes not" is that its use as an object key will differ based on engine support. What's the benefit of using a |
|
Mainly the idea that a signal token should be unique/difficult to reproduce. It's the inverse of the edge case of interop between instances or implementations. Definitely an edge case, but, for example, someone passes strings through a channel. If they're coming from user input, an unscrupulous user could pass '@@csp/CLOSED' to shut down the pipeline. An implementer would need to either add @@csp/CLOSED to the list of inputs to escape, additional handlers for false positives, or always wrap the strings in an object (granted, it's unlikely they wouldn't be wrapped in the first place). This thread has brought up some points in favor of using a string that I hadn't thought of, though. Thoughts? |
|
The fact is that the I vote for a normal namespaced string, as it's a lot more predictable. |
|
What's unpredictable about a |
It's printed very strangely in various consoles |
|
Why does "how a debugger/development console chooses to display it" matter? |
|
A developer will see this a lot while debugging? I think we're worrying too much about something that will never happen in production, like I said several other libraries use namespaced strings and it's ok. EDIT: and it's more predictable regarding multiple versions or other libraries that want to re-use it, since equality will always return true |
|
So, having thought about this again, it really comes down to a trade-off: with a pure string approach, a developer would need to escape incoming string data to avoid a minor "Bobby Tables" vulnerability, or - with a non-string approach like the string object - a developer would need a helper function for doing comparisons across platforms or implementations. Something like var toString = Object.prototype.toString;
function isClosed(token) {
return token == csp.CLOSED && toString.call(token) === '[object String]';
}Either way would probably necessitate a helper function to simplify things for a user, the question is which way you want to go. Personally I still think it'd be better to go with the string object approach, or even Still, escaping would also make it impossible to close the channel via input alone, since you wouldn't be able to tell whether the Granted, I realize the odds are pretty slim for someone to want to cause this by typing in that string. Anyway, if you still want to go the string approach, I'm happy to update this PR accordingly to save you the effort. |
|
I would prefer We would need to add proper tests for browsers and multi-version tests for npm though. |
|
However, it will work fine if when Symbols aren't available in the environment, it falls back to an |
+1 for symbols |
|
I think it's generally the consensus that Symbol.for would be the ideal solution - the problem is that they can't be polyfilled in a complete manner, and regular Symbols have the same limitation as Object strings. So if everyone is comfortable with Symbol.for, and a fallback that won't work cross-realm without a more complex checking method, that's fine. Is that an acceptable level of inconsistency though? It'd look like: try {
csp.CLOSED = Symbol.for('@@csp/CLOSED');
if (typeof csp.CLOSED !== 'symbol') throw new TypeError('Symbols not supported.');
// probably wouldn't really use arrow fns
csp.isClosedToken = token => token === csp.CLOSED;
} catch(e) {
csp.CLOSED = Object('@@csp/CLOSED');
// this would work cross-realm.
csp.isClosedToken = token =>
(token == csp.CLOSED && toString.call(token) === '[object String]');
} |
|
That solution LGTM, especially since you're providing the "is" function. |
…k to Object strings in environments that lack native symbols.
…ush CLOSED to pending takes"
jaawerth
changed the title
|
Merged in the discussed revisions; it'll now default to using a global Symbol via Symbol.for, which will work cross-realm, and defined an The only downside is the potential complexity of having to use the isClosedToken function when potentially working across realms. |
…documented csp.isClosedToken
|
Bump. Looks like |
| closedToken = Object('@@csp/CLOSED'); | ||
| } | ||
| return closedToken; | ||
| })(); |
There was a problem hiding this comment.
Wouldn't it make more sense to write this as:
var CLOSED = (function() {
try {
return Symbol.for('@@csp/CLOSED');
} catch(e) {
return Object('@@csp/CLOSED');
}
})()
Removes unnecessary variables.
There was a problem hiding this comment.
I suppose. Doesn't really matter. Style preference.
There was a problem hiding this comment.
That works fine, but provides zero value as the engine can optimize those out anyways.
There was a problem hiding this comment.
Yeah, they'll be pretty much equivalent, and I don't mind changing it if people find that version more readable - to me it's six of one, half dozen of the other. Tough to remember a year later, but I likely wrote it with a variable after initially going without the IIFE, and adding it in to avoid a function-wide deopt.
|
Following up on @tejasmanohar's comment, is this an acceptable approach or is there still some question as to what the preferable approach would be? Would maintainers still prefer to use a different Symbol fallback approach, like a normal namespaced string so |
Addresses #60 .
Used a String object:
Object('csp::CLOSED')rather than an empty object. The result iscsp.CLOSED === csp.CLOSED:truecsp.CLOSED === 'csp::CLOSED':falsecsp.CLOSED == 'csp::CLOSED':truewhich I think makes sense for this use-case. This allows null values to be passed into channels without accidentally closing them.
It also (as pointed out to me by @ljharb) allows people to use csp.CLOSED as an object key for mapping errors and the like.