3.2+ Cleanup function is not called within specific component

[brenner-company]

For a package I'm working on I'm creating a floating-ui modifier & component (using https://floating-ui.com/) to attach floating elements to a trigger/reference. Everything seems to work fine, but I've encountered an issue today: as documented within ember-modifier, you need to return a cleanup function to remove any added event listeners, etc when the component gets unloaded.

When I test this with just the modifier, everything works as expected: the cleanup function gets called when eg. navigation away from a page with said modifier. But when I use the component version I've created, the cleanup function is not being called.

I can easily check this by creating the following setup: two toggles that add & remove the modifier and component version. When I open the event listener inspector in Chrome and click the toggle a few times I see the following results.

Modifier version:

Component version:

(The event listeners are not getting removed here)

I've put up an example of that code at: https://codesandbox.io/s/cool-morning-9moikc?file=/app/modifiers/au-floating-ui.js (component version can be found under app/components/au-floating-ui.hbs & app/components/au-floating-ui.js)

Could the specific structure of the component be an issue here or have I stumbled upon a bug?

By the way: my code is pretty similar (regarding structure) to https://github.com/CrowdStrike/ember-velcro but with less customisation. So, the same issue would also be present in there.

[brenner-company]

If, by any chance, the CodeSandbox example preview gets stuck on Initializing sandbox container, this can be fixed by toggling (hide + show) the preview pane itself:

[NullVoxPopuli]

👋 thanks for the reproduction!

any chance the reproduction can be reduced to bare minimum? there is a lot going on in there, and some things looked not wired up correctly :-\

[apellerano-pw]

I think I have a bead on this, but it requires Ember 3.27+ for the (modifier) helper, so I can't easily produce a twiddle to share.

My theory is that if you're specifying a modifier conditionally, and then remove the template from the DOM while the modifier is still present, the modifier's destructor function won't be called.

{{!-- my-component.hbs --}}
<div
  {{(if
    @myFlag
    (modifier "my-modifier")
  )}}
>stuff</div>

The syntax above is based on Chris Krycho's article on conditional modifiers
https://v5.chriskrycho.com/journal/conditional-modifiers-and-helpers-in-emberjs/

@myFlag defaults false; you set it true - modifier applies; and then <MyComponent/> is removed from the DOM, you will not see the modifier's destructor get called. However if you set @myFlag back to false, the modifier's destructor will run and then you can remove the component from the DOM.

In my case I observed this in a rendering test. Forgive me, our tests are in mocha (actually, pray for me)

it('never calls the destructor', async function () {
  this.set('myFlag', false);
  await render(hbs`
    <MyComponent @myFlag={{this.myFlag}}/>
  `);
  this.set('myFlag', true);
  // the functional modifier has run
  await clearRender();
  // the modifier's destructor has not run!
});

it('calls the destructor', async function () {
  this.set('myFlag', false);
  await render(hbs`
    <MyComponent @myFlag={{this.myFlag}}/>
  `);
  this.set('myFlag', true);
  // the functional modifier has run
  this.set('myFlag', false);
  // the modifier's destructor runs
  await clearRender();
});

I see similar syntax used in brenner-company's example:

floater=(if this.reference (
    modifier
      this.floaterModifier
      this.reference
      this.arrow
      defaultPlacement=@defaultPlacement
      options=@options
      destroy=this.destroy
  ))

I'm not actually sure whose bug this is! ember-modifier? glimmer-component? ember-source? some handlebars template lib?

[NullVoxPopuli]

This is super helpful, @apellerano-pw.

I had a bit of a tangent while trying to reproduce though, for example,
the following code:

  <div {{ (if @enabled (modifier testModifier))}}>stuff</div> {{! strict mode }}

Generates a VM Error, so I'll be debugging that later. had to update somedeps

On stackblitz, I'm not using strict mode, so was able to make progress -- I found that this works as you would expect it to (destructing every time the if statement becomes false).

<fieldset><legend>inline</legend>
  <div {{ (if @enabled (modifier 'my-modifier' "inline")) }}>content</div>
</fieldset>

So now, what's left is to conditionally yield like in your floater example:

<fieldset><legend>yielded</legend>
  <YieldModifier @condition={{@enabled}} as |myModifier|>
    <div {{myModifier}}>content</div>
  </YieldModifier>
</fieldset>

But I was still unable to reproduce the behavior you describe.
The console output matches what I would expect:

constructing via let 1
constructing inline 2
constructing yielded 3
--
destructing via let 1
destructing inline 2 
destructing yielded 3
-------------
constructing via let 4
constructing inline 5 
constructing yielded 6
--
destructing via let 4 
destructing inline 5 
destructing yielded 6

Do you have an open source repo that I could run with minimal examples (as in my stackblitz, or in your test snippets?) -- I'd like to be able to run something "known to be goofy", so that maybe I don't miss some detail when I create my own reproduction.

Thanks!!!

[apellerano-pw]

@NullVoxPopuli I wasn't able to run your stackblitz, but I made a small repro repo.

https://github.com/apellerano-pw/ember-modifier-613-repro/blob/main/tests/integration/modifiers/add-hello-class-test.js

I believe the bug has to do specifically with removing (conditionally applied) modifiers from the DOM; the behavior works fine when setting the conditional true/false and the modifier remains in the DOM. It might be possible to trigger in your stackblitz if you did something like this:

{{#if this.enabled}}
  <TestCondition @enabled={{this.enabled}}/>
{{/if}}

{{!--
It would depend on how Ember propagates state changes.
Does the `(if)` helper destroy `<TestCondition>`, and never pass the new value down to it?
Or does the component update before it's destroyed by the `(if)` change?
--}}

For my repro, I use the test environment. The modifier adds a class to body. The destructor should remove this class. This works fine when specifying the modifier directly, even using the (modifier) helper. However once you conditionally render the modifier, the test fails because the modifier's destructor doesn't run.

P.S. My repro is Ember 3.28 and ember-modifier 2.x since that is the exact environment I noticed the issue in.

[NullVoxPopuli]

wasn't able to run your stackblitz

I'm using firefox, and my blitz works, but yours does not 🤔
I tried loading your project here:
https://stackblitz.com/github/apellerano-pw/ember-modifier-613-repro/?file=app%2Fmodifiers%2Fadd-hello-class.js

One thing I noticed is that you're using:

• ember-source@3.28
• ember-cli-htmlbars@v5
• ember-auto-import@v1
• ember-modifier@v2

After upgrading these packages to:

• ember-source@4.11
• ember-cli-htmlbars@v6
• ember-auto-import@v2
• ember-modifier@v4
• webpack@v5

And I was able to boot your project here: https://stackblitz.com/edit/github-eh3tjp?file=package.json
(I also removed some unused dependencies to make install faster)

I see that your first test fails: it never calls the destructor.

adding / removing a class isn't a great indicator of the described behavior because destruction is async (extra complexity to test for when modifying classList, and if the modifier is re-enabled, the destructor could remove the class added in the next modifier run).

I was able to reproduce your test with something similar, yet not messing with the element:

  test('assert calls the destructor', async function (assert) {
    this.set('enabled', false);
    this.set(
      'myModifier',
      modifier((element) => {
        assert.step('myModifier set up');
        return () => assert.step('myModifier cleaned up');
      })
    );
    await render(hbs`
      <div {{(if this.enabled (modifier this.myModifier))}}>
        qwerty
      </div>
    `);
    
    assert.verifySteps([]);
    this.set('enabled', true);
    await settled();

    await clearRender();

    assert.verifySteps(['myModifier set up', 'myModifier cleaned up']);
  });

I can confirm the test passes if I change the render call to:

    await render(hbs`
      <div {{this.myModifier}}>
        qwerty
      </div>
    `);

so this at least means we have a legit reproduction!!! 🎉
Thanks for providing this -- it's quite helpful!

I have a hunch where the fix would need to go, but I'm not certain.
Current hypothesis is that the VM needs to associate destroyable children on values as applied in modifier-syntax, regardless of how they got there (hopefully accounting for if as well as other conditional ways of rendering the primitives)

[Techn1x]

My theory is that if you're specifying a modifier conditionally, and then remove the template from the DOM while the modifier is still present, the modifier's destructor function won't be called.

Just commenting here to say I've hit this issue today, exactly as described. I essentially had code like this, where I was using the modifier helper to do a conditional modifier, a condition which was somewhat tied to also having the element removed from the DOM. When this.selectedPreview went falsey, I noticed the destroy function of the modifier was not run (we need the destroy to be run so that the phaser game cleans up correctly, stops its audio etc. - it's not just enough that the element is removed from DOM)

{{#if this.selectedPreview}}
  <Modal>
    // loading spinner etc.
    // currentManifest is a trackedFunction
    <div
      {{(if this.selectedPreview.currentManifest.value
        (modifier myModifier this.selectedPreview.currentManifest.value)
      )}}
    />
  </Modal>
{{/if}}

The syntax above is based on Chris Krycho's article on conditional modifiers
https://v5.chriskrycho.com/journal/conditional-modifiers-and-helpers-in-emberjs/

Funnily enough I was following the same guide 😄

---

Glad to see there's a repro in this PR - awesome detective work!!

If anyone needs a workaround, this worked for me;

{{#if this.selectedPreview.currentManifest.value}}
  <div {{myModifier this.selectedPreview.currentManifest.value}} />
{{else}}
  <div />
{{/if}}