bug: ref callback order

[George-Payne]

Prerequisites

• I have read the Contributing Guidelines.
• I agree to follow the Code of Conduct.
• I have searched for existing issues that already report this problem, without success.

Stencil Version

2.17.4

Current Behavior

When using a ref callback, new refs are given before old refs are nullified.

For example, if you change a key on an element, the ref will be called with the new div before it is called with null

my-component.tsx

import { Component, h, State } from '@stencil/core';

let i = 0;
const nextKey = () => `key-${i++}`;

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.css',
  shadow: true,
})
export class MyComponent {
  @State() currentKey: string = nextKey();

  componentDidLoad() {
    setInterval(() => {
      this.currentKey = nextKey();
    }, 3_000);
  }

  render() {
    return (
      <div ref={this.captureDiv} key={this.currentKey}>
        {this.currentKey}
      </div>
    );
  }

  private captureDiv = (r: HTMLDivElement | null) => {
    console.log(this.currentKey, r);
  };
}

Will give:

key-0 <div>​key-0​</div>​
key-1 <div>​key-1​</div>​
key-1 null
key-2 <div>​key-2​</div>​
key-2 null
key-4 <div>​key-4​</div>​
key-4 null
key-5 <div>​key-5​</div>​
key-5 null
// etc

This is potentially problematic, as if you track a single ref:

private captureDiv = (r?: HTMLDivElement) => {
    this.divRef = r;
};

The reference will be set to null after the new ref is given.

Expected Behavior

Old refs should be nullified before new refs are given.

key-0 <div>​key-0​</div>​
key-1 null
key-1 <div>​key-1​</div>​
key-2 null
key-2 <div>​key-2​</div>​
key-4 null
key-4 <div>​key-4​</div>​
key-5 null
key-5 <div>​key-5​</div>​
// etc

---

For reference, equivalent code in react will give null before the next ref is given:

https://jsfiddle.net/yk23w1dj/16/

"key-0", "DIV"
"key-1", "null"
"key-1", "DIV"
"key-2", "null"
"key-2", "DIV"
"key-3", "null"
"key-3", "DIV"

// etc

Steps to Reproduce

1. clone reproduction repo
2. switch to branch https://github.com/George-Payne/stencil-bug-reproductions/tree/ref-key-order
3. npm install
4. npm run dev

Open in browser and view console:

key-0 <div>​key-0​</div>​
key-1 null
key-1 <div>​key-1​</div>​
key-2 null
key-2 <div>​key-2​</div>​
key-4 null
key-4 <div>​key-4​</div>​
key-5 null
key-5 <div>​key-5​</div>​
// etc

3. Note that the "new" ref is given before the previous one is nullified.
4. Same behaviour for production build: npm run prod

Code Reproduction URL

https://github.com/George-Payne/stencil-bug-reproductions/tree/ref-key-order

Additional Information

Possibly related to: #3253

[alicewriteswrongs]

Hi @George-Payne thanks for filing this issue! I believe you are right that this is related to #3253, what you are describing is basically the same issue, with the caveat that the behavior described in #3253 is what happens if you don't set the key prop on an element which is moving while you're describing what happens if the key is mutated.

At present what Stencil does with key attributes, why they are helpful for the Stencil component runtime, and so on isn't really documented, which is unfortunate. However, having just read through a bunch of the relevant source code while working on #3253 I can say that at present our virtual DOM implementation does depend on key props on elements being static in order to get any guarantees about correctly following children through re-orders and so on. I am planning to write some documentation about this soon to clarify this better.

Is mutating these keys something you need to do for some reason? I am wondering what the use-case is, just because my feeling at present is that the intended way to use key attrs is that they have to be the same across renders to get any guarantees about finding components that move across re-renders. Using a key attr like this recently fixed a related issue with ref getting nulled on the Ionic Framework (ionic-team/ionic-framework#25838).

Anyhow - just trying to understand better what you're trying to do and what the exact issue is. Thanks again for filing the issue!

[George-Payne]

Hi Alice,

Is mutating these keys something you need to do for some reason?

Passing a different key forces a recreation of an element, so you can start with a clean slate. I was mostly using it as an easy way to reproduce the issue.

Here's a reproduction that doesn't use keys, which, though still contrived, is maybe more like something someone might write:

my-component.tsx

import { Host, Component, h, Listen, State } from '@stencil/core';

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.css',
  shadow: true,
})
export class MyComponent {
  @State() wrap: boolean = false;

  componentDidLoad() {
    setInterval(() => {
      this.wrap = !this.wrap;
    }, 3_000);
  }

  @Listen('click')
  handleClick() {
    console.log(this.innerDiv);
  }

  renderInner() {
    return <div ref={this.captureDiv}>{'hello'}</div>;
  }

  render() {
    return (
      <Host>
        {this.wrap ? (
          <article>
            <h1>{'article'}</h1>
            {this.renderInner()}
          </article>
        ) : (
          this.renderInner()
        )}
      </Host>
    );
  }

  private innerDiv: HTMLDivElement | null;
  private captureDiv = (r: HTMLDivElement | null) => {
    console.log(this.wrap, r);
    this.innerDiv = r;
  };
}

The console gives:

false <div>​hello​</div>​
true <div>​hello​</div>​
true null
false <div>​hello​</div>​
false null
true <div>​hello​</div>​
true null
false <div>​hello​</div>​
false null

As the refs are cleaned up after the new ref is called, after the first wrap this.innerDiv is always set to null.

(I'm not expecting the div inner div to be reused or anything, as the render tree has changed. Just that refs are unrefed before the new refs come in.)

---

Again, for reference, here is a react equivalent, showing the expected behaviour https://jsfiddle.net/3a47xzjd/5/

false, "DIV"
true, "null"
true, "DIV"
false, "null"
false, "DIV"
true, "null"
true, "DIV"

[alicewriteswrongs]

Ok that makes sense. The issue reproducing without setting a key is basically what #3253 is all about. My current take on the situation is that Stencil's VDom implementation is, at present, not correct because you shouldn't need to set keys in order to get the correct behavior for setting a ref like this, both because ideally there would be a good heuristic for matching up VDom children across re-renders and because we should provide a guarantee that code relating to taking old elements off the DOM should run before code relating to adding new elements to it.

Two points:

1. In React my limited understanding is that setting key props is basically something which allows the React runtime to optimize the handling of children and run a faster algorithm for detecting which VDom children at time t are still around (but possibly in a different order) at time t+1. If you don't set a key, however, React will still produce correct behavior (refs consistently pointing to the right element, DOM in-line with the VDOM state, etc) and basically it will just be a bit slower (to a possibly-unimportant degree) and print a little warning. The problem we have in Stencil has to do with the fact that if static keys are not set the algorithm that Stencil uses for matching up children from time t to t+1 is a heuristic and does not guarantee that if a node is present at both times, but in a different position, it will always be found¹. If you're interested in digging into this I recently wrote some documentation of how Stencil handles updating children which might be of interest. So anyhow, because there are issues w/ how Stencil figures out when to re-render children, it is relatively easy to create situations where children are re-rendered needlessly, causing all the lifecycle stuff for those nodes to happen.
2. So there's a problem with detecting when children should and should not be re-rendered. This in turn can, if those children have refs on them, expose another problem, which is that Stencil does not currently guarantee that, within a virtual-DOM re-render cycle, the 'cleanup' type stuff from getting rid of old DOM nodes and whatnot will run after the 'startup' type stuff associated with creating new VNodes and their associated concrete DOM nodes. This is basically what is happening in the case where a ref ends up being null inappropriately - the teardown code which runs after the associated DOM element is going to be removed can, in certain situations, run after the startup code for a new equivalent DOM element being created. In particular, these things will happen based on the element order within the children, so sometimes the issue will be apparent when things are in a certain order but not show up otherwise.

Anyhow, that is my understanding of where this is at presently. I believe in practice Stencil users can avoid the problem of inappropriately-null refs by using static, consistent keys on any elements which a) are rendered as children of other elements and may change position / order and b) have a ref anywhere in their subtree. This isn't exactly a full and complete way to fix the problem but I think users who do this in practice will have no trouble.

A difficulty that the Stencil team is dealing with is that the virtual DOM code is fairly complex and none of us are the original authors of it, so it is somewhat time consuming for us to understand and change it. Still, we are game to try! As part of addressing #3253 we plan to 1) document the existing behavior and the use-case for setting key attrs to avoid the issue and 2) investigate setting compile- or run-time warnings that not setting key attrs may cause issues if we can figure out a way to detect those situations.

In the longer term we may look at rewriting the VDom code to provide better guarantees and produce correct behavior in this case without having to use key attrs, but I think I can't commit to any kind of timeline for that at present.

Footnotes

1. If you set a static key I believe we can provide a guarantee that the matching child can be found, but I haven't done extensive investigation of edge cases there, it's possible that they exist as well ↩

[alicewriteswrongs]

@George-Payne just had a bit more of a think on this and I'm going to go ahead and label it to ingest into our backlog, I'm going to keep #3253 open for now for tracking some more work related specifically to the key attribute stuff (like documenting it and so on) and keep this issue for tracking work on improving the virtual DOM implementation itself

[George-Payne]

Thanks for extensive reply, and the docs you linked were interesting.

I believe in practice Stencil users can avoid the problem of inappropriately-null refs by using static, consistent keys on any elements which a) are rendered as children of other elements and may change position / order and b) have a ref anywhere in their subtree.

In the second example, the issue can't be resolved by adding a key, due to the changing the nodes position in the tree.

my-component.tsx (with key)

import { Host, Component, h, Listen, State } from '@stencil/core';

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.css',
  shadow: true,
})
export class MyComponent {
  @State() wrap: boolean = false;

  componentDidLoad() {
    setInterval(() => {
      this.wrap = !this.wrap;
    }, 3_000);
  }

  @Listen('click')
  handleClick() {
    console.log(this.innerDiv);
  }

  renderInner() {
    return (
      <div ref={this.captureDiv} key={'static-key'}>
        {'hello'}
      </div>
    );
  }

  render() {
    return (
      <Host>
        {this.wrap ? (
          <article>
            <h1>{'article'}</h1>
            {this.renderInner()}
          </article>
        ) : (
          this.renderInner()
        )}
      </Host>
    );
  }

  private innerDiv: HTMLDivElement | null;
  private captureDiv = (r: HTMLDivElement | null) => {
    console.log(this.wrap, r);
    this.innerDiv = r;
  };
}

my-component with key console output.

false <div>​hello​</div>​
true <div>​hello​</div>​
true null
false <div>​hello​</div>​
false null
true <div>​hello​</div>​
true null
false <div>​hello​</div>​
false null

Because the tree goes from:

my-component
└── div { key: "static-key" }
    └── #text "hello"

to:

my-component
└── article
    ├── h1
    │   └── #text "article"
    └── div { key: "static-key" }
        └── #text "hello"

and updateChidren only compares children, so doesn't see that "static-key" has moved down the tree.

The call order then looks something like this:

• [ div { key: "static-key" } ] -> [ article ]
  • addVnodes:
    • article
      • for newVNode.$children$
        • createElm h1
        • createElm div { key: "static-key" }
          • updateElement
            • setAccessor
              • newValue (call ref with html node)
  • removeVnodes
    • div { key: "static-key" }
      • callNodeRefs (call ref with null)

So as addVnodes is called before removeVnodes, the ref function is called with the added node before the ref with null is called.

In React my limited understanding is that setting key props is basically something which allows the React runtime to optimize the handling of children...

(In my even more limited understanding) react is doing the same thing (just comparing children). but because (the equivalent of) removeVnodes is called before (the equivalent of) addVnodes, you clean up your old ref before you get the new one, so it isn't an issue.

[George-Payne]

• Virtual DOM patching algorithm based on Snabbdom

Coludn't find an equivalent issue in Snabbdom, but:

• Production setAccessor() function based on Preact

This seems to be the same thing in 'preact':

preactjs/preact#2194

With the fix:

preactjs/preact#2217

But this doesn't seem to map directly to anything in stencil, as far as I can see.

[alicewriteswrongs]

That all makes sense to me, and you're right that currently there's no real way to workaround this issue if nodes are moving up and down the tree like that (the bit of the updateChildren algorithm that looks at the keys only looks at the children from one node to another).

I think we could easily implement something along the lines of the fix in that Preact PR.

Thanks for finding and sharing all these things! My feeling at present is that this issue is probably present in a fair few virtual DOM implementations and fixed only in some (like Preact, React, etc).