create-ref doesn't seem to clean up after element is destroyed

[swastik]

<div>
  <button class="p-2 ml-2" {{on "click" this.addItems}}>Add 100 items</button>
  <button class="p-2 ml-2" {{on "click" this.clear}}>Clear</button>
</div>

{{#each this.items as |item|}}
  <div {{create-ref (concat "item-" item)}}>
    {{item}}
  </div>
{{/each}}

import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';

export default class Test extends Component {
  @tracked items: number[] = [];

  @action addItems() {
    this.items = [
      ...this.items,
      ...Array(100)
        .fill(0)
        .map((_, i) => i),
    ];
  }

  @action clear() {
    this.items = [];
  }
}

I have some code like above. When I add 100 items, then clear them, I'm expecting that those elements won't show up in memory, but I'm still seeing them as detached elements. Here's a screenshot from Edge:

[lifeart]

Hi @swastik, thank you for report!
Could you try to check if this line is executed: https://github.com/lifeart/ember-ref-bucket/blob/master/addon/modifiers/create-ref.js#L25?

[swastik]

We are on an older version (3.x.x). It seems like that's called when elements are inserted into the DOM:

Screen.Recording.2023-10-05.at.11.08.28.mov

But not when they are destroyed:

Screen.Recording.2023-10-05.at.11.09.00.mov

[swastik]

In the latest version (5.0.4), it seems like it is called the way you expect, however, we still end up with what seems like detached elements in memory at the end. I'm not sure if it's this addon causing it or something deeper down the stack?

Screen.Recording.2023-10-05.at.11.15.52.mov

[lifeart]

@swastik I think we could try to create reproduction test with access to this of component, to verify that defined element props return null after list removal / component destruction.

// component for test
{
  constructor() {
    this.args.sendContext(this);
  }
}

// test logic
let context = null;
this.onSetContext = (ctx) => {
  context = ctx;
}
this.render('<Component @sendContext={{this.onSetContext}} />`)

// here we do some mutations
// here we check for `context.ref_1`, `context.ref_2`, etc to check is it alive or not

[swastik]

Thanks! I added a test:

  test('it renders', async function (assert) {
    let context = null;
    this.sendContext = (ctx) => {
      context = ctx;
    };

    await render(hbs`<XButton @sendContext={{this.sendContext}} />`);

    console.log(context);

    console.log(context.item);
    await click('.add-items');
    console.log(context.item);

    await click('.clear');
    console.log(context.item);
    await this.pauseTest();
  });

item on the component is @ref('item0') item

Now:

• When I click add, then clear, the DOM nodes are removed, but the item reference is present
• When the test completes and the application (I'm assuming) is torn down, the reference is also gone

Here's a screenshot of the flow:

[swastik]

So I think what's happening is this destructor:

    registerDestructor(this, () => {
      this.cleanMutationObservers();
      this.cleanResizeObservers();
      getNodeDestructors(this._element).forEach((cb) => cb());
    });

… is not clearing keys from the bucket. if I add a delete and call it like so:

    registerDestructor(this, () => {
      this.cleanMutationObservers();
      this.cleanResizeObservers();
      bucketFor(this._ctx).delete(this._key)
      getNodeDestructors(this._element).forEach((cb) => cb());
    });

… and in createBucket, roughly this:

    delete(name) {
      delete this.bucket[name];
      delete this.keys[name];
    },

That seems to remove references to the element etc. Does that make sense, or is it not intended to work like that?

[lifeart]

@swastik yep, all right! It should work, in addition, we could try to use WeakRef #46 in element getter, to avoid "referencing", anyway, I'm happy we able to track it down and feel free to create related PR, for latest/3.xx versions.

[lifeart]

Hi @swastik, could you check #57 ?

[lifeart]

@swastik, here is branch with v3 fix, could you also check? (#59)

[swastik]

@lifeart thank you, I'll take a look this week. We shipped a patch with WeakRef with patch-package to production last week and that seems to have worked. It's the same approach effectively that you've got in your PR! 🙇🏼

[swastik]

@lifeart I tried that out locally and it did work!

[lifeart]

HI @swastik!
Thank you for report and testing!
v3.1.1 and v5.0.5 released