aknuds1 / knockout-isotope

Isotope binding for Knockout
8 stars 5 forks source link

ko-isotope has problems when filtering arrays #1

Closed jonjaques closed 11 years ago

jonjaques commented 11 years ago

I've prepared a test-case and a stack trace below.

I believe the cause is that knockout is replacing the contents of the array rather than modifying it, and isotope is holding onto an index reference.

Will try to hack on this later tonight, but in the mean time I wondered if you had any ideas. Thank you for open sourcing this btw, saved me a lot of time.

Uncaught TypeError: Cannot read property 'index' of undefined jquery.isotope.js:587
  $.Isotope._getSorter jquery.isotope.js:587
  sortFn jquery.isotope.js:573
  $.Isotope._sort jquery.isotope.js:583
  $.Isotope.remove jquery.isotope.js:828
  (anonymous function) jquery.isotope.js:1387
  jQuery.extend.each jquery-1.6.4.js:660
  jQuery.fn.jQuery.each jquery-1.6.4.js:274
  $.fn.isotope jquery.isotope.js:1375
  beforeRemove knockout-isotope.js:44
  ...
aknuds1 commented 11 years ago

Thanks for trying Knockout-Isotope and bringing this issue to my attention, I am looking into it, but can't dedicate very much time today. What would accelerate the process though, is if you could fork my project and write a Jasmine testcase which reproduces the error. Then issue a pull request. Otherwise I'll try to write a Jasmine test myself, as I systematically track requirements through this :)

aknuds1 commented 11 years ago

I've found the reason for the problem, Knockout clears jQuery data for each node before it calls the beforeRemove callback. This is kind of weird since the beforeRemove callback is responsible for removing nodes, and Isotope relies on jQuery data when removing an element (node). I got things working by disabling the call to jQuery.cleanData, but we have to figure out what is the right approach.

jonjaques commented 11 years ago

@aknuds1, I really appreciate your help with this. Looking at 3466 in knockout, I just replaced ko.cleanNode with an identity function. This probably isn't the best way to go about things, but it serves my purposes.

// Next remove nodes for deleted items (or just clean if there's a beforeRemove callback)
ko.utils.arrayForEach(nodesToDelete, options['beforeRemove'] 
  ? function(value){ return value; }  // was ko.cleanNode
  : ko.removeNode
);

The only thing I could think of would be to return a preference of some sort in the binding, and check for that inside of knockout.

The larger issue I suppose is that we need a hacked version of knockout in the first place. It seems to me that because of how DOM-centric Isotope is, makes it really hard to work with a stock version of KO (as I'm sure you are aware).

aknuds1 commented 11 years ago

I'm planning on asking on the Knockout mailing list how to resolve this case. It just seems weird to me to delegate deleting nodes to the beforeRemove callback, and still handle cleaning up of 3rdparty jQuery data yourself.

Regarding hacked Knockout in the first place, I'm also going to investigate whether the beforeAdd callback can be made standard.

aknuds1 commented 11 years ago

I have issued a pull request to Knockout, of a branch where KO will not call jQuery.cleanData if a beforeRemove callback is supplied.