emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.46k stars 4.21k forks source link

`Infinite revalidation` bug error thrown in willDestroyElement calls if we set any parent properties inside it in Ember versions higher than 5.5.0 #20707

Open tamil-arasu849 opened 1 week ago

tamil-arasu849 commented 1 week ago

We recently upgraded our ember application from version 3.28 to v5.9.0. After upgrade some cleanup operations using willDestroyElement calls in ember component throws infinite revalidation bug error, but when we use willDestroy method or registerDestructor function, this issue doesn't occur.

We found that this issue is present in ember versions starting from v5.6.0. But upto v5.5.0 this issue isn't there. I've tried to reproduce the bug in the repo https://github.com/tamil-arasu849/ember-lifecycle-hook-bug-repo.

The app will throw the below error when toggling the child component. But downgrading the version to v5.5.0 or lower fixes this issue image

This bug breaks many of our existing components

NullVoxPopuli commented 1 week ago

Thanks for the reproduction!

after looking through your code, it seems like you don't want destruction to auto-track. A work-around (we'll need to defer to @chancancode to know if this is a bug or not), is to change: https://github.com/tamil-arasu849/ember-lifecycle-hook-bug-repo/blob/main/app/components/example.js#L10

@action
toggleChild() {
  setProperties(this, { showChild: !this.showChild });
}

to

@action
async toggleChild() {
  await 0
  setProperties(this, { showChild: !this.showChild });
}
tamil-arasu849 commented 1 week ago

But this change breaks many of our existing components, so had to downgrade to v5.5.0. Also we can't do this handling in all places right! image

NullVoxPopuli commented 1 week ago

Discussed in discord, an alternative is to switch to willDestroy, from willlDestroyElement, which will also help in migration to Glimmer components (since glimmer components don't have willDestroyElement)

tamil-arasu849 commented 1 week ago

Won't it cause any issue if we use willDestroy in classic components? @NullVoxPopuli @chancancode

NullVoxPopuli commented 1 week ago

why would it? -- it exists in classic components https://api.emberjs.com/ember/5.9/classes/Component/methods/willDestroy?anchor=willDestroy

kategengler commented 4 days ago

I believe switching to willDestroy would only be an issue where you need access to the element in willDestroyElement.

tamil-arasu849 commented 2 days ago

Do we expicitly set the element of a classic component to null | undefined after willDestroyElement or will it be garbage collected when component instance it freed?

NullVoxPopuli commented 2 days ago

it'll be cleaned up, all handled by the framework <3

tamil-arasu849 commented 2 days ago

Yeah. I'm asking can we able to access the HTML element in the willDestroy hook or will be set as null | undefined by the framework after the willDestroyElement hook call

NullVoxPopuli commented 2 days ago

I don't recall, that'd be a good thing to try -- what is this.element

kategengler commented 1 day ago

I don't believe the element is available in willDestroy -- otherwise we would not have needed willDestroyElement as a hook.