knockout / knockout

Knockout makes it easier to create rich, responsive UIs with JavaScript
http://knockoutjs.com/
Other
10.45k stars 1.52k forks source link

Instead of `instanceof` use `toString` to solve cross-frame/context issues #1307

Open alFReD-NSH opened 10 years ago

alFReD-NSH commented 10 years ago

So I use jsdom to run my binding handlers unit tests. And I realized checked binding is failing to recognize that the value of the binding is an array but it was treating it as a boolean, so checked binding was checking all my checkboxes instead of one. I checked knockout source and found the problem was coming from this line: ko.utils.unwrapObservable(valueAccessor()) instanceof Array,

The problem is that my unit tests is in one context(the main nodejs script context) and knockoutjs source script is evaluated in another context(done by jsdom). And the Array constructor is different in both contexts. Same issue happens when if the array came from another frame in the browser.

This problem has been described here: http://perfectionkills.com/instanceof-considered-harmful-or-how-to-write-a-robust-isarray/

To replicate the problem on browser you can try this:

var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
xArray = window.frames[window.frames.length-1].Array;
var arr = new xArray(1,2,3); // [1,2,3]

// Boom!
arr instanceof Array; // false

// Boom!
arr.constructor === Array; // false

As a fix, we can add a util function called isArray:

knokcout.utils.isArray = Array.isArray || function isArray(o) {
  return Object.prototype.toString.call(o) === '[object Array]';
};

But arrays are not the only things that are checked with instanceof. I found this other checking if object is primitive or not: https://github.com/knockout/knockout/blob/c59104cde3eaeb2120203a17a19d2fd620df1055/src/subscribables/mappingHelpers.js#L27

and also somebody asked for this before: https://github.com/knockout/knockout/pull/184#issuecomment-24156563

Note that most of popular utilities libraries(underscore, lodash, sugar, jquery) use this method to check the type.

JamesThorpe commented 10 years ago

We're also seeing this issue, but in ko.toJS on an object originally from a parent frame - internally in the mapJsObjectGraph function it uses instanceof to determine if the rootObject is an array or not. As above, using the toString check for '[object Array]' fixes the issue.

JamesThorpe commented 10 years ago

More details of how we were experiencing the issue on stackoverflow: http://stackoverflow.com/questions/24890168/ko-tojs-converting-array-to-object/

aufula commented 7 years ago

please fix this