Element modifier destruction broken in animated-if with Ember 3.17

[Turbo87]

see ember-animation/ember-animated#202

Since I'm not sure if the issue is in ember-animated or in the new Ember release I'm opening an issue here too.

[makepanic]

I noticed a similar issue when using the will-destroy render modifier.

Given two components, one using @ember/component (Foo) and one using @glimmer/component (Bar).

Component impl:

export default class Foo extends Component {
  _didInsert = (element) => {
    console.log('glimmer', '{{did-insert}}', element, document.querySelector('#yield-bar'), document.querySelector('#outside-yield'));
  }
  _willDestroy = (element) => {
    console.log('glimmer', '{{will-destroy}}', element, document.querySelector('#yield-bar'), document.querySelector('#outside-yield'));
  }
  willDestroy(){
    console.log('glimmer', 'willDestroy', document.querySelector('#yield-bar'), document.querySelector('#outside-yield'));
  }
}

with their template:

<div
  {{did-insert this._didInsert}}
  {{will-destroy this._willDestroy}}
>
  <div>{{yield}}</div>
</div>

and using them in an ember template:

{{#if this.renderComponents}}
  <Foo><h3 id="yield-foo">yield Foo</h3></Foo>
  <Bar><h3 id="yield-bar">yield Bar</h3></Bar>
  <h3 id="outside-yield"></h3>
{{/if}}

I can see a different lifecycle of will-destroy:

3.17:

3.16:

It seems like the will-destroy modifier lifecycle changed and it's called after the element was already removed from the DOM.

(I know that one shouldn't query the DOM that way but there might be addons that rely on elements existing when will-destroy is invoked)

[pzuraq]

@makepanic the change in destroy order there is actually expected. Classic components had used a different destruction queue before the VM upgrade which added a fair amount of complexity, and was actually unnecessary and took them out of the normal destruction flow (and ordering).

The VM upgrade fixed this, and like you said folks shouldn't be relying on the ordering of willDestroy specifically (whereas they should be able to rely on the ordering of willDestroyElement, and that hasn't changed), which is why we felt comfortable making the change.

[makepanic]

Is there a hook for glimmer components to execute when it will destroy but is still in DOM?

I noticed that basic-dropdown (maybe cc @cibernox ) (see template ) does manual DOM manipulation in the will-destroy modifier. There are probably others that will break with 3.17 so we might need to migrate them.

[pzuraq]

@makepanic ah, sorry, I didn't read your comment closely enough. I thought you were discussing the ordering of the willDestroy hooks, which changed (the Classic component's comes last).

We've been discussing the modifier issue, there isn't a resolution just yet because the new behavior is actually what was spec'd in the original modifier RFC. But, the nature of modifiers implies they should have a willDestroyElement hook of some sort, and it's likely folks were using the destroyModifier hooks in the managers for that purpose. Will prioritize the discussion for that.

[cibernox]

This change is indeed surprising. The name willDestroy totally makes one think that whatever you run there will happen before DOM nodes are destroyed.

[pzuraq]

Per the modifier manager RFC:

May or May Not

• be called in the same tick as DOM removal

I think a lot of folks who were writing modifiers did not realize this, but it makes sense given what willDestroy semantically is about. It's really about cleaning up JavaScript, e.g. preventing memory leaks and other work that can be done lazily. Long term, the goal is to begin scheduling this work to happen during idle time, so it does not block rendering, since it shouldn't actually affect rendering.

Modifiers, though, likely need a way to clean up before DOM removal given their purpose. In addition, the {{will-destroy}} modifier actually had a contradiction in it's the spec - it both guaranteed running before removal and had the above language (copy-pasted from the other RFC 😬 )

[Turbo87]

hmm... given the above, how does the on modifier call removeEventListener on element if that element is potentially not there anymore? 🤔

or is the element still there, but just not attached to any parent node anymore?

[pzuraq]

The element is still there, it has just been removed from the DOM. In many cases, this is all that is needed, like for instance with removeEventListener - detaching the component from the DOM will cause the browser to automatically remove and garbage collect it once the element is fully removed.

[Turbo87]

okay, thanks for the clarification.

it looks like my issue above is slightly different though, because it seems that the will-destroy modifier is not being called at all.

[pzuraq]

Yes, absolutely. I haven't had a chance to dig in here, but I'm tracking and will be taking a look when I can.

[miguelcobain]

I also noticed this when trying to access this.element.parentElement inside the willDestroy hook of a modifier.

[scottmessinger]

I'm guessing this is related to #18855 (comment)

[andreyfel]

@Turbo87 is it still an issue in 3.18/3.19?

[chbonser]

@andreyfel I'm seeing this with PaperMenu while upgrading to Ember 3.20.

[LucasHill]

I concur this is still happening in 3.20.

[elfin-sbreuers]

I know it is a bit outdated already but stumbled accross that when I was wondering if the willDestroy can be blocked for period of time.

The {{did-insert}} modifier is often exemplified for the addition of a class that handles an animation:

https://github.com/emberjs/ember-render-modifiers#example-adding-a-class-to-an-element-after-render-for-css-animations

That is a nice feature. It would be great to have a symmetrical operation that allows the teardown while still using the css to animate out. I would have liked to see that {{will-destroy}} or the destruction function in custom modifiers support the usage of a promise that would prevent the destruction for as long as the promise remains unresolved.

From a software development perspective it is not the cleanest approach (since you would need the animation duration in two places), but for the user experience it would be nice to use the inverse animation path by using the same CSS animation.