Transition does not call onUnload about ember-onbeforeunload HOT 12 OPEN

So I think that either README should state that one needs to make sure any code within onUnload needs to be blocking or that onUpdate is changed so that it would be onUnload(resolve, reject) to help people get correct behavior.

Likely the README update. The latter would be confusing since window.onunload won't allow anything async to run in full (AFAIK). onUnload is meant for when the application is going to be torn down, not to be fired between routes.

But you raise an interesting point, perhaps we need a hook for handling transitions as well. So transitions between routes could be async, but a transitions away from the app needs to be sync.

I think a new hook should be introduced for onRouteUnload vs onAppUnload.

from ember-onbeforeunload.

Thanks for reporting this @matthewvalimaki - can you throw together a quick twiddle that shows this behavior for us?

here's a twiddle with the addon pre-installed:

https://ember-twiddle.com/af013400c74fc407a45e74327ed9f9cb

thank you!

from ember-onbeforeunload.

@blimmer done, see https://ember-twiddle.com/e4683bc6bd03a0a01698886ed7daa002.

from ember-onbeforeunload.

Just a bit off-topic but related to onUnload execution and the fix I am proposing. In my use case when user says "yes" I need to revert bunch of database changes and only after that transition. Problem is that onUnload by default does not use Promise and so asynchronous things become somewhat problematic.

So I think that either README should state that one needs to make sure any code within onUnload needs to be blocking or that onUpdate is changed so that it would be onUnload(resolve, reject) to help people get correct behavior.

If we were to change to onUpdate(resolve, reject) then the first thing would be to transition.abort(), call onUnload and then transition.retry().

from ember-onbeforeunload.

Forgot to mention that transition should be given to onUnload in case there's further logic I want to do. Also it might be a good idea to hand the model like with others as well. While I can get it myself from transition it would be code replication vs. having it supplied.

from ember-onbeforeunload.

The way I understood README was that this was for route transitions as well, so I would prefer your suggestion to introduce new hooks to clarify intent.

from ember-onbeforeunload.

@jasonmit Do you think you'll be able to add onRouteUnload easily? or do you need help?

from ember-onbeforeunload.

@dja pretty easy. The logic is in place, just need to split the onUnload hook up into two hooks and call the correct respective hook depending on what occurred. As for help, would love the help :)

from ember-onbeforeunload.

Hi! I wanted to see if this fix was planning on being implemented anytime soon? It seems like that PR above would work for my use case.

from ember-onbeforeunload.

@devups lets get the PR green and I’ll cut a beta. I need to analyze if this is a breaking change. I think this behavior could also be configured so that it’s an opt in instead of all or nothing. How does that sound?

from ember-onbeforeunload.

@tylerturdenpants Yeah, that sounds awesome. Ty!

from ember-onbeforeunload.

@devups Do you mind taking over the PR work? I’m simply too busy.

from ember-onbeforeunload.