Transition does not call onUnload

[matthewvalimaki]

@jasonmit first thank you for this trivial looking yet complicated support for detecting when transitioning away from current view. On surface it looks easy but due to browser differences it is a bit pain.

When I navigate to a completely different route (not sub-route) and answer "yes" to the dialog onUnload is not called. I believe this is because to the browser it's not unloading anything. I was able to fix this by adding this.onUnload() after https://github.com/jasonmit/ember-onbeforeunload/blob/2c8c7bbfa72c08a3a3bf2b49ee3c7a8307fffc80/addon/mixins/confirmation.js#L98. So there I added else { this.onUnload(); }.

[blimmer]

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!

[matthewvalimaki]

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

[matthewvalimaki]

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().

[matthewvalimaki]

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.

[jasonmit]

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.

[matthewvalimaki]

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.

[dja]

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

[jasonmit]

@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 :)

[ghost]

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.

[tylerturdenpants]

@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?

[ghost]

@tylerturdenpants Yeah, that sounds awesome. Ty!

[tylerturdenpants]

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