Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
alFReD-NSH opened this issue Feb 10, 2014 · 3 comments
Open

Comments

@alFReD-NSH
Copy link

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:

var canHaveProperties = (typeof rootObject == "object") && (rootObject !== null) && (rootObject !== undefined) && (!(rootObject instanceof Date)) && (!(rootObject instanceof String)) && (!(rootObject instanceof Number)) && (!(rootObject instanceof Boolean));

and also somebody asked for this before: #184 (comment)

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

@JamesThorpe
Copy link

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
Copy link

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

@alFReD-NSH alFReD-NSH changed the title Instead use instanceof use toString to solve cross-frame/context issues Instead of instanceof use toString to solve cross-frame/context issues Jul 23, 2014
@mbest mbest modified the milestone: Not assigned Dec 2, 2016
@aufula
Copy link

aufula commented Dec 21, 2016

please fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants