added internal _isArray function and replaced 5 instances of constructor... #303
Conversation
…tor === Array checking
| @@ -70,6 +70,16 @@ | |||
| return keys; | |||
| }; | |||
|
|
|||
| var _isArray = function(maybeArray){ | |||
There was a problem hiding this comment.
var isArray = Array.isArray || function(maybeArray){
return {}.toString.call(maybeArray) === "[object Array]";
};There was a problem hiding this comment.
is that considered better style? I just wanted it to be clear in the code that it was our own function. Using the shim can make it unclear.
There was a problem hiding this comment.
I'd prefer to name it with an underscore _isArray, as @brianmaissy suggests, to make it clear that it's our version of it, but I also prefer @michaelficarra's implementation which avoids an extra function call in the case that Array.isArray exists.
There was a problem hiding this comment.
ah good point. i'll commit again.
@michaelficarra sorry, I read your comment too quickly and thought you were suggesting Array.isArray ||=...
What do you think about {} vs Object.prototype? I'm inclined to prefer the extra clarity of Object.prototype even though it's longer.
There was a problem hiding this comment.
Less statically analysable. The interpreter has to resolve both Object (in the scope chain) and prototype (in Object' s prototype chain). With {}, you know immediately to look at the Object.prototype object.
Either way, it doesn't really matter. I was just showing you that there is a reason to choose one over the other, though, if you really care.
|
+1 vote, because tests with the sandboxed-module are failing with code using async. |
|
+1 as well, this breaks tests that override constructors for introspection |
|
+1 please merge ! |
|
+1 |
|
👍 This fixes #392 and #393, and it's already been mentioned that this fixes #3, #179, #344 and #832. Why hasn't this been merged and these issues/pull requests closed? Fixes #3! This is an old, old outstanding bug. Dead easy fix. No new releases for 11 months. This is a ~7,000 star project on GitHub. Has it been abandoned? 200 outstanding issues? Does anyone know of an up-to-date and actively maintained fork? |
|
@jmdobry have you found a maintained fork ? |
|
@caolan @brianmaissy can we get this merged? |
|
@neoziro No, might just have to start one. |
|
sorry guys, i haven't been contributing to this project for a while. try emailing caolan |
|
fixed in d5d0efb |
|
@caolan Nice! Thank you |
|
@caolan thanks |
|
@caolan great thanks |
* waterfall call in another context * parallel call in another context * series call in another context
... === Array checking
Solves issues #302 and #179.
Replaces pull request #294, which should be closed.