Implement Iterable Support #4
Conversation
|
Awesome! Sorry I dropped the ball on responding to this, but I'm taking a look and of course we want to get this added :) |
| @@ -211,3 +225,48 @@ function convertTimezone(tz) { | |||
| } | |||
| return false; | |||
| } | |||
|
|
|||
| function isArrayLike(val) { | |||
| if (!iterableSupport && Array.isArray(val)) { | |||
There was a problem hiding this comment.
I assume it is not expected that the state of iterableSupport would change at runtime, is that correct? If so, it is possible to simply split apart this functions into two versions instead of checking the iterableSupport boolean constantly? I am seeing a bit of a performance dip in production from this PR, and I'm not 100% sure where it is coming from (it's running in an environment where iterableSupport === false), but when I simply changed these functions to only have the iterableSupport === false code, it helped bring it pretty much all the way back up to previous levels.
| map.forEach(function(val, key) { | ||
| object[key] = val; | ||
| }); | ||
| // if custom toString was implemented, attach to new object |
There was a problem hiding this comment.
Forgive me for not fully understanding Map objects, but what is the purpose of this? Is it even performent to constantly create new Map() over and over again?
| @@ -25,6 +26,15 @@ test('SqlString.escapeId', { | |||
| assert.equal(SqlString.escapeId(['a', 'b', 't.c']), "`a`, `b`, `t`.`c`"); | |||
| }, | |||
|
|
|||
| 'sets are turned into lists': function() { | |||
| if (!iterableSupport) return; | |||
There was a problem hiding this comment.
Typically it is best that if a test is going to be skipped, it's simply just not added to the object so it doesn't show up like there is a test happening. At least, the pattern with utest, which does not have a skip mechanism.
| var set = new Set(); | ||
| set.add('a'); | ||
| set.add('b'); | ||
| var sql = SqlString.format('? and ?', set); |
There was a problem hiding this comment.
Wait, what? I don't think this makes sense. I don't think a top-level Set object should act like an array, because isn't a Set order independent? The top-level is very much order-dependent.
|
Hi @krmannix do you think you'll get a chance to take a look at the review comments? |
|
Hi @dougwilson - will try to do this upcoming weekend! Sorry I've dropped the ball on responding - thanks for taking the time to review it. |
|
@dougwilson going to close this, and if I can find a cleaner implementation I'll reopen. Thanks again for the time you took reviewing! |
Attempts to implement support for es6 iterables like
MapandSet, to fix #1422.Setshould behave like anArray,Mapshould behave like anObject.