Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Replace constructor === Array checks with safer Array.isArray #179

Closed
wants to merge 1 commit into from

5 participants

@cliffano

MDN Object constructor reference page ( https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Object/constructor ) states that constructor property can be changed and it is not always safe to believe in constructor function.

I found an issue when using the latest version of async with the latest version of sandboxed-module ( https://github.com/felixge/node-sandboxed-module ) where async.parallel thinks that tasks variable is not an array even though it is actually an array. This is caused by "tasks.constructor === Array" returns false, while Array.isArray(tasks) returns true.
This is very likely to be caused by sandboxed-module issue 13 ( felixge/node-sandboxed-module#13 ) which changes the value of constructor property, and is still an open issue.

Granted that this is not an async bug, but it would be better if async doesn't rely on constructor property which can be modified by other modules, and uses Array.isArray instead.

@caolan
Owner

Please bear in mind that async is also a browser library and will need a browser-compatible fallback for this.

@qubyte

MDN suggests using the trick

Object.prototype.toString.call(array) === '[object Array]';

A function can be aliased to Array.isArray if available, and otherwise this. This solution is guaranteed to work by the 5th edition standard. However, various versions of IE fall down when an array comes from a different frame, in which case the test will return false because you get '[object Object]'. If arrays from other frames with IE is not a great concern, then the above may be an acceptable shim.

@caolan
Owner

@qubyte that's the trick I'd normally use for shimming Array.isArray, so it's acceptable to me at least ;)

@dougwilson

Also, @qubyte's comments regarding Arrays from other frames would have already not worked with the constructor tests, as Array in one frame would not actually be the same Array as another frame, so I'd say it's a non-issue regarding replacing obj.constructor === Array constructs.

@cliffano

Closing this issue since #303 has a more comprehensive fix.

@cliffano cliffano closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 2 deletions.
  1. +2 −2 lib/async.js
View
4 lib/async.js
@@ -448,7 +448,7 @@
async.parallel = function (tasks, callback) {
callback = callback || function () {};
- if (tasks.constructor === Array) {
+ if (Array.isArray(tasks)) {
async.map(tasks, function (fn, callback) {
if (fn) {
fn(function (err) {
@@ -480,7 +480,7 @@
async.series = function (tasks, callback) {
callback = callback || function () {};
- if (tasks.constructor === Array) {
+ if (Array.isArray(tasks)) {
async.mapSeries(tasks, function (fn, callback) {
if (fn) {
fn(function (err) {
Something went wrong with that request. Please try again.