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

Wrapping fastForEach in If causes removeNodes to fail #50

Open
rposener opened this issue Apr 9, 2018 · 5 comments
Open

Wrapping fastForEach in If causes removeNodes to fail #50

rposener opened this issue Apr 9, 2018 · 5 comments

Comments

@rposener
Copy link

rposener commented Apr 9, 2018

Hi came across this when trying to use this binding (which works great by the way).

If you wrap the fastforeach binding in a it will in many cases, cause an error, as the delete function runs after the nodes are removed from DOM, so nextSibling does not return the next item. This causes pendingDeletes to have the first Node, then gets a null as the second which has 2 problems;

  1. it throws during ko.removeNode cleanup which could prevent other code from running.
  2. it is effectively a leaky handler as it then doesn't cleanup those nodes/handlers/etc.
@cervengoc
Copy link
Collaborator

Hi @rposener, could you please put together a jsfiddle or similar to show us how to reproduce this issue? I've used this binding for years now in many complex situations and didn't experience this behavior.

Thanks

@rposener
Copy link
Author

rposener commented Apr 9, 2018

Hi @cervengoc - it took me a little to bring in enough functionality to get it to replicate in a simple enough manner. Here is an example: jsFiddle . Maybe you can point out something else that I'm doing that is causing the issue. Works OK with normal foreach.

@rposener
Copy link
Author

@cervengoc or @brianmhunt any ideas on this? I'm discovering that this is leaving some other things not properly disposed after changing.

@rposener
Copy link
Author

Just an update on this - what I've done to work around this in my application is only use the if: binding to delay the init of the template binding until I've rendered (in code) the template. I've modified my approach such that I have a component that never has to re-render my template. I had gotten into the pre-rendering of a template programmatically as I had nested for-each bindings (a table of sorts) that was doing like 2,000+ items, with some number of columns (25 was not uncommon) - which amounted to 50,000 template renders. I found that pre-processing/rendering of the column template, then just cycling the rows 2,000 times was much more performant - especially in Edge.

This can be closed if you want, but it's still an issues if the fastForEach is within a binding that controls descendentBindings (such as if).

@brianmhunt
Copy link
Owner

Thanks @rposener Glad you solved the issue, and thanks for reporting — good to know about this (even if we can't get it resolved immediately)

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

No branches or pull requests

3 participants