Using the .id getter in the isSoftMatch can be problematic when another js frameworks override it

[dombesz]

The Problem

I'm using angular frontend components that are being sent to the frontend via turbo.js, morphed with Idiomorph. The result of the morphing will insert duplicate elements in case of angular components. This happens because, the isSoftMatch function uses the id as comparison as (!oldElt.id || oldElt.id === newElt.id) and angular can override the id getter.

In my case, the angular components coming via turbo do not have an html id attribute, but they have a @Input id = "default-id" property in the component definition instead.. Because of this, angular will dynamically assign an id to the component, and calling oldElt.id will return "default-id", although no html id attribute was provided in the component template.

This is problematic because the newElt component is yet to be inserted to the DOM and angular did not initialise it component yet. As a result, calling newElt.id will not be "default-id", but it will be the id provided as the html attribute on the response, which is an empty string in my case.

Proposed solution

In order to avoid the interference of initialized vs not initialized angular components, I propose to compare the id html attributes directly by calling oldElt.getAttribute("id") instead of oldElt.id in the isSoftMatch method:

 function isSoftMatch(oldNode, newNode) {
  // ok to cast: if one is not element, `id` and `tagName` will be undefined and we'll just compare that.
  const oldElt = /** @type {Element} */ (oldNode);
  const newElt = /** @type {Element} */ (newNode);

  return (
    oldElt.nodeType === newElt.nodeType &&
    oldElt.tagName === newElt.tagName &&
    // If oldElt has an `id` with possible state and it doesn't match newElt.id then avoid morphing.
    // We'll still match an anonymous node with an IDed newElt, though, because if it got this far,
    // its not persistent, and new nodes can't have any hidden state.
    (
      (!oldElt.getAttribute("id")) ||
      (oldElt.getAttribute("id") === newElt.getAttribute("id"))
    )
  );
}

I can provide a PR if this turns out to be a confirmed bug. Thanks!