Should we allow calling the close callback more than once?

[lolmaus]

Hi!

@herzzanu and me have investigated a flaky test in a huge app.

After some frustrating investigation we have discovered, that the cause of test failures was that a modal's @close callback was called twice.

The frustration was caused by trhee parts:

• everything is async, so the stack trace points at a promise that had been caught by Ember Backburner or something, and the stack trace does not point at the code of the promise callback;
• we have a chain of ember-concurrency tasks threading through multiple components; a modal is opening a modal, etc;
• tests never failed locally when the browser tab was focused; tests would sometimes fail when the tab was not focused. 🤯

The specific error turned out to be "calling set on destroyed object", with stack trace pointing at this line: https://github.com/mainmatter/ember-promise-modals/blob/v4.0.0/addon/components/modal.js#L148

The solution on our side was to make sure the modal was not being closed twice.

But I wonder, if this should even be a developer's concern? Why not let developers close the modal more than once?

At the very least, we could wrap the set into if (!this.isDestroying && !this.isDestroyed). Or we could make the close callback return early if it is called more than once.

[nickschot]

My thoughts:

Simplest solution is probably to use some kind of boolean (probably not isDestroying since I think that's a reserved thing on EmberObjects?) that prevents the entire close sequence that allows for the animation to run from running multiple times. I don't think it can be interrupted and we probably don't want that possibility anyway.

Ideally it'd also have a dev-time assertion when it does happen to be running multiple times so that we spot the issue easier. Something like "can't close modal that is already closing".