Support ECMAScript 2015 Set in for block #759 #760
Conversation
Synchronous only.
| @@ -106,6 +106,10 @@ exports.isArray = Array.isArray || function(obj) { | |||
| return ObjProto.toString.call(obj) === '[object Array]'; | |||
| }; | |||
|
|
|||
| exports.isSet = function(obj) { | |||
| return ObjProto.toString.call(obj) === '[object Set]'; | |||
There was a problem hiding this comment.
What about polyfills? [object Object]...
There was a problem hiding this comment.
What about obj && obj[Symbol.toStringTag] == 'Set'? Does not require shims to patch Object#toString (I would argue that they should, and most of them do), but requires Symbol polyfill.
|
I don't like it, it turned out unreadable, duplication and bad tests |
| } | ||
|
|
||
| this.emitLine('} else if(runtime.isSet(' + arr + ')) {'); { | ||
| v = this.tmpid(); |
There was a problem hiding this comment.
do we need a different tmpid here? why not use the same one? the is-array and is-set clauses are mutually exclusive.
There was a problem hiding this comment.
cargo cult programming
|
I think you're being too hard on yourself :-) The for loop code was already hard to read (code-generation code in general is hard to read, and nunjucks generally is too). Your PR follows the same patterns as the existing code (don't worry about some duplication, prioritize speed over size) and makes sense to me. What's wrong with the tests? |
|
Looks like it's failing on Node 0.10, probably because it doesn't support |
yes |
|
Actually the code supports anything that happens to support the iterator protocol. But to be pragmatic, and support old browsers, why not use Array.from directly. The only caveat I can think of is providing a string, which would iterate over the individual characters. But then again, by then your in la la land anyways. The code could be simplified as |
|
@maxnordlund steal it, please 😀 |
|
So Christmas happened, but I finally got around to doing this. Check it out and give me some feedback, pretty please? |
|
This is done I think, ping @fdintino |
|
It is. I'll wait until I merge in the develop branch and put out a release before I close out this issue though. |
Synchronous only.