refactor(datetime): use key to preserve calendar body ref node

[alicewriteswrongs]

This fixes an issue with the Datetime component where in certain
circumstances a ref on an element rendered by the datetime component
would end up erroneously set to null, necessitating a yucky
document.querySelector based workaround.

The issue arises because of how Stencil's virtual DOM implementation
handles reconciling the children of old and new nodes, and in particular
how it deals with situations in which the order of children changes but
some of the nodes are conserved.

The way this was affecting the datetime component was that in certain
circumstances a method which returns a <div> with a ref would change
position in the children of another node. In particular, this can happen
if the presentation prop changes, causing the return value of the
renderDatetime function to change in turn. The salient changes to
the return value of that function which would change look like
going from:

[
  this.renderCalendar(mode),
  this.renderTime()
]

to

[
  this.renderTime(),
  this.renderCalendar(mode)
]

renderCalendar in turn calls renderCalendarBody which returns the
<div> with the offending ref on it.

Anyhow, in reconciling these changed children when re-rendering the
component things can go wrong. Stencil's algorithm in this case does a
"best effort" at identifying nodes which should be kept between
re-renders, but it does not do an exhaustive check unless nodes have a
key attr. So, more or less, what was happening was that in cases where
the <div> with the ref on it was going to be included in the next
rendered output from the component Stencil would nonetheless remove the
'old' node and create a new identical one for the next render. Then
there is also a race condition where the new node would be created
before the old one is removed, causing the ref to be briefly set to
the new value (when the new VNode is created) and then reset to null
(when the old one was destroyed).

The fix is to set the key attribute on the <div> which is the
parent of the <div> with the ref. This allows Stencil to do an
exhaustive check when reconciling old and new children in order to
not needlessly throw away the old VDom node, and gets rid of the
behavior where the ref would end up set to null erroneously.
It's possible (but I think unlikely) that this change will result
in slightly less GC pressure, but I did no testing of that.

closes stenciljs/core#3253
FW-901

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
• Build (npm run build) was run locally and any changes were pushed
• Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

There is a bug in Stencil which this fixes (stenciljs/core#3253).

Basically, there was an issue where switching the presentation prop on the Datetime component would erroneously set a ref stored on the component to null. This fixes that by setting a key attr on the relevant div so that Stencil's VDom child reconciliation / update algorithm can do a better job of finding nodes that are the same between renders.

Issue #3523

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

[alicewriteswrongs]

just pointing one thing out

[alicewriteswrongs]

I went ahead and remove this little method, but kept the various if-falsy-early-return checks around the component that look like this:

const calendarBodyRef = this.calendarBodyRef
if (!calendarBodyRef) {
  return;
}

so as to minimize churn (like an alternative would be to replace those intermediate calendarBodyRef variables with references to this.calendarBodyRef, but unfortunately I don't think typescript will narrow the type of properties like that after an early return so they'd all have to have ! put at the end which is yucky)

[averyjohnston]

Amazing 🤯 Testing this fix out, things look great on my end. I had a couple questions:

1. Your explanation mentions that the key attribute should be set on the same element as the ref, but here it's actually set on the ref's parent (.datetime-calendar vs .calendar-body). Was that intentional?
2. Is there a general guide floating around anywhere for when to use the key attribute, especially as it pertains to diagnosing and fixing this sort of issue? (Or will the VDom guide you're working on address that?)

[alicewriteswrongs]

To your questions: 1. I think my wording was a little fuzzy there. The place to set the `key` attr is on whatever element is going to be changing position from one render to another within the children of its parent, so I think on `.datetime-calendar` is the right spot. 2. I don’t think using the key attr is documented anywhere at present. As far as I can tell it basically works the same way the key attr does in React in similar situations, allowing the Vdom implementation to detect children which should be retained across renders in more situations. I believe that the key attr checks have been in Stencil for a long time but we’re never documented, possibly they were just copied over when the vdom implementation was done. I am going to look into both adding documentation to explain when to use it and try to see if we could print a compile- or run-time warning in Stencil when the lack of a key attr could lead to this type of problem.

…

On Mon, Aug 29, 2022 at 8:24 AM Amanda Johnston ***@***.***> wrote: ***@***.**** commented on this pull request. Amazing 🤯 Testing this fix out, things look great on my end. I had a couple questions: 1. Your explanation mentions that the key attribute should be set on the same element as the ref, but here it's actually set on the ref's parent (.datetime-calendar vs .calendar-body). Was that intentional? 2. Is there a general guide floating around anywhere for when to use the key attribute, especially as it pertains to diagnosing and fixing this sort of issue? (Or will the VDom guide you're working on address that?) — Reply to this email directly, view it on GitHub <#25838 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPLRHGUJ774NC2XO257NHLV3TI2PANCNFSM57X67FKQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[averyjohnston]

Perfect, thank you 👍 I'm gonna add the team back on as a reviewer but I think just one more pair of eyes as a sanity check would be good, whoever gets to it first.

[liamdebeasi]

Thank you!!

[averyjohnston]

Merged; thanks again!