Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.17] WillDestroy not called after a component is destroyed if that component was rendered from adding items to a list #18797

Closed
jakesjews opened this issue Mar 5, 2020 · 11 comments · Fixed by glimmerjs/glimmer-vm#1065 or #18838

Comments

@jakesjews
Copy link
Contributor

jakesjews commented Mar 5, 2020

If a component is rendered for each item in an each block then any components created by adding new items to the bound list will not call their willDestroy hooks. The issue affects both classic and glimmer components.

The bug is demonstrated at https://github.com/jakesjews/ember-will-destroy-bug/blob/master/tests/integration/components/test-comp-test.js. The console logs whenever each component is created or destroyed. The first test logs out two creates and two destroys which is expected but the second test logs two creates and only one destroy.

@jakesjews jakesjews changed the title [3.17] WillDestroy not called on components rendered from adding items to a list [3.17] WillDestroy not called after a component is destroyed on components rendered from adding items to a list Mar 7, 2020
@boris-petrov
Copy link
Contributor

I just hit the same issue.

@jakesjews - did you find a fix? If not, I'll have to downgrade to 3.16 because that's a major problem for us.

@jakesjews
Copy link
Contributor Author

@boris-petrov unfortunately I have not. I downgraded our app to 3.16.

@jakesjews jakesjews changed the title [3.17] WillDestroy not called after a component is destroyed on components rendered from adding items to a list [3.17] WillDestroy not called after a component is destroyed if that component was rendered from adding items to a list Mar 18, 2020
@pieter-v
Copy link
Contributor

Same problem here

@manufitoussiwit
Copy link

Same problem here, too. I went crazy with this :-). I'm downgrading to 3.16.3.

Copy link
Member

rwjblue commented Mar 29, 2020

Yeah, sorry about this y'all. Recent changes have this fixed and released in v3.17.2.

@seidelman
Copy link

I think I still have a problem with when willDestroy() is actually called.

My application has a list that is no longer rendered after the last element is removed:

{{#if list}}
  <div>
     {{#each list as |item|}} 
        <Component @item={{item}} {{will-destroy some-action}} /> 
     {{/each}}
  </div>
{{/if}}

There is logic outside of this template to add and remove items to the list as application runs.

In 3.16.6

willDestroy() is called immediately after an item is removed from the list. I think this is expected behavior.

In 3.17.1

willDestroy() is not called at all.

In 3.17.2

willDestroy() is called for all removed items after {{#each}}...{{/each} block itself is no longer rendered. This means that there may be a very long delay between component removal from the DOM and willDestroy().

@simonihmig
Copy link
Contributor

willDestroy() is called for all removed items after {{#each}}...{{/each} block itself is no longer rendered. This means that there may be a very long delay between component removal from the DOM and willDestroy().

I can confirm the observation here, and this is still happening in Ember 3.18!

You can see this very well in the Ember inspector. See the following debugging experiment, captured from an actual app:

Kapture 2020-05-20 at 20 07 54

The field "rafters count" represents the length of the array being iterated over. The grey buttons are what is rendered in the each loop for each item. And on the right you see the matching component instances (<BsButton>), which are not properly destroyed. After reducing the array from 5 to 4 items, you can still see 9 component instances in the inspector (didn't bother to setup a proper key for the {{each}}, so there are the 5 to-be-destroyed components plus the 4 new ones)

In the actual use case where this came up, the willDestroy hook not only cleaned things up, but had to do some important side-effects. So this is not a purely aesthetic issue, but actually broke behavior.

@rwjblue sorry, but I think this needs to be reopened!

@waissbluth
Copy link

@simonihmig can you confirm that this still happens in v3.18.1, after #18941 was merged?

@rwjblue
Copy link
Member

rwjblue commented May 20, 2020

Ya, there is also another issue fixed WRT rerenders within an {{#each that hasn't been released just yet (was fixed in #18982).

@simonihmig
Copy link
Contributor

can you confirm that this still happens in v3.18.1, after #18941 was merged?

@waissbluth Oh, great catch! I though we were already on the latest stable release 🤦‍♂️.

So yes, works as expected again! Sorry for the noise here!

Copy link
Member

rwjblue commented May 20, 2020

Awesome, thanks for confirming that it is fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants