fix remove hook during pantry storage

[MichaelWest22]

When we remove a node that contains preservable id's the node is placed in the pantry to be reused later. However if there are also nodes that we want to preserve with a before remove hook then these will now fail to be preserved as we expect because they will now be in the pantry in error. To avoid this issue we can remove ids from idMap consideration once the id'ed element has been moved out by moveBeforeById. This means the remaining content will then not have any preservable id's left in it when it gets eventually processed and removed and then the before remove hook will fire correctly and not remove the preserved node as expected.

[botandrose]

@MichaelWest22 I took a look at this, and I think the test isn't really representing the issue well. Specifically, the beforeNodeRemoved hook is preventing all removals, not just the #preserve-me element. So I've reduced the test case down to this, which I think captures the problem a bit better:

  it("returning false to beforeNodeRemoved can preserve descendents of deleted nodes", function () {
    let initial = make(`<div><div><input id="preserve-me"></div></div>`);
    Idiomorph.morph(initial, `<div></div>`, {
        callbacks: {
          beforeNodeRemoved: (node) => node.id !== "preserve-me",
        },
      },
    );
    initial.outerHTML.should.equal(`<div><div><input id="preserve-me"></div></div>`);
  });

This fails with and without the proposed changes.

[MichaelWest22]

That test is not valid sorry. The node being removed is not being checked in that callback. This has always been how it works in old versions. When a callback wants to preserve a node with a preserve attribute for example it has to check all its children to see if they contain the preserve attribute and then return if they want that node tree preserved. If there are other children nodes with no preserve attributes they would also be preserved then as well so it is up to the callback to code to remove or deal with these nodes if required

[botandrose]

@MichaelWest22 I think we agree on all of that. When I first discovered this issue, I didn't do a great job of representing it, but ultimately this was the issue I was surprised by. When I realized that it wasn't a regression, I concluded we could ignore it, at least for the time being.

But now I'm gathering that you think there's a separate issue aside from the one I was concerned about? I'll take another look.

[MichaelWest22]

Yeah I found there was cases where we are pantrying preserved nodes if they used to have a id in their tree. Which is stupid as they can often be preserved if requested by the callback. So if we remove id's from the map as they are being preserved we can avoid this bad edge case and do the right thing more often. But the other issues you found wirh children nodes with callbacks not firing on parent node removal is not related at all and an issue that we can address with better hooks in the client side

[botandrose]

@MichaelWest22 Gotcha, I see it now! Great catch. I'm going to refactor it a bit then merge, so we can get it into the upcoming release.

[botandrose]

Closed by 0ab4f9b