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

New beforeRemove behavior can break non-removed items #1903

Merged
merged 2 commits into from
Oct 20, 2015

Conversation

mbest
Copy link
Member

@mbest mbest commented Oct 13, 2015

While re-testing the examples included with #1256, I discovered that when removing items from an array, following items might be "cleaned" so that their bindings no longer worked. This happens when the nodes for an item aren't all removed at the same time and fixUpContinuousNodeArray exits with the "undefined scenario".

@ThomasMichon
Copy link

I'm glad you found this, because I was seeing issues in my project related to fixUpContinuousNodeArray encountering a null value for the mappedNodes array. However, I could not figure out what the repro case might be.

@mbest
Copy link
Member Author

mbest commented Oct 13, 2015

@ThomasMichon I'm not sure if this is the same problem as you're describing because I didn't ever see a null mappedNodes.

…eted nodes even when some are removed (including the first and last nodes).
@mbest
Copy link
Member Author

mbest commented Oct 13, 2015

@rniemeyer Could you please review these changes?

@@ -275,8 +282,10 @@ ko.utils = (function () {
while (current !== last) {
continuousNodeArray.push(current);
current = current.nextSibling;
if (!current) // Won't happen, except if the developer has manually removed some DOM elements (then we're in an undefined scenario)
return;
if (!current) { // Won't happen, except if the developer has manually removed some DOM elements (then we're in an undefined scenario)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the change above (new rule B), we can probably remove this check since the last node should always be there. What do you think, @rniemeyer?

@rniemeyer
Copy link
Member

@mbest - I'lll review as soon as I can

@rniemeyer
Copy link
Member

@mbest this looks good and I agree that it should be safe to remove the check that you commented on.

mbest added a commit that referenced this pull request Oct 20, 2015
New beforeRemove behavior can break non-removed items
@mbest mbest merged commit a03cd3f into master Oct 20, 2015
@mbest mbest deleted the 1903-beforeRemove-update branch October 20, 2015 22:04
@mbest
Copy link
Member Author

mbest commented Oct 20, 2015

Thanks, @rniemeyer. I've merged this now.

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

Successfully merging this pull request may close these issues.

None yet

3 participants