lifeart / ember-ref-bucket

This is list of handy ember primitives, created to simplify class-based dom workflow
MIT License
36 stars 14 forks source link

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

Closed swastik closed 11 months ago

swastik commented 11 months ago
<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:

image
lifeart commented 11 months ago

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 commented 11 months ago

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

https://github.com/lifeart/ember-ref-bucket/assets/1569205/aeb92e3b-fdac-4bf7-8fff-552670b2284d

But not when they are destroyed:

https://github.com/lifeart/ember-ref-bucket/assets/1569205/25be8d49-ceb9-42cb-b37f-9c9bc6da4cbf

swastik commented 11 months ago

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?

https://github.com/lifeart/ember-ref-bucket/assets/1569205/0f793a73-a6dc-48e2-9ff7-633b18b7fbee

lifeart commented 11 months ago

@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 commented 11 months ago

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:

Here's a screenshot of the flow:

image
swastik commented 11 months ago

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 commented 11 months ago

@swastik yep, all right! It should work, in addition, we could try to use WeakRef https://github.com/lifeart/ember-ref-bucket/issues/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 commented 11 months ago

Hi @swastik, could you check https://github.com/lifeart/ember-ref-bucket/pull/57 ?

lifeart commented 11 months ago

@swastik, here is branch with v3 fix, could you also check? (https://github.com/lifeart/ember-ref-bucket/pull/59)

swastik commented 11 months ago

@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 commented 11 months ago

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

lifeart commented 11 months ago

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