Implemented onNavigate

[PatrickG]

Fixes #1040

It works exactly like the example in my comment + you can return a promise inside onNavigate.

[antony]

I think the functionality is great - and certainly something that would be useful.

Whether the syntax conforms is a different question. I'm wondering if something at the app level:

<App on:navigate={someFunction}> would be more appropriate, though of course this runs the risk of making it a Svelte level change, and since Svelte doesn't interact with routing, this might not work.

[PatrickG]

I don't think this should be handled by Svelte.

Also, the App-Component is created and injected by the Sapper runtime. That means you can not use on:navigate since you never write <App /> in your own code.
Or do you mean as an event that can be listened to on every Svelte-Component? That would probably not resolve the issue, because most of the time you need to listen to this event from inside of a Page-Component.

[pngwn]

Does subscribing to the page store (which changes when you navigate) not solve this problem?

[PatrickG]

How should that prevent the navigation?

[lowi]

@pngwn You can't cancel the navigation by subscribing to page store.
I am using @PatrickG fork to show a dialog "You didn't save your changes. Do you want to stay on the page or lose your changes?" - which is not possible with store subscription.

[webfrank]

When this will be merged? It will solve lot of use cases where you have to control page navigation.

[benmccann]

This could use some docs added under site/content/docs/ explaining the usage. That will also make the code easier to review. E.g. right now I'm not quite sure what the return value of onNavigate is used for

I wonder if using the browser's beforeunload would be a better way to handle this than introducing canNavigate since it also covers the case where the user is closing the tab

My other question is whether we can have a method that's called before navigation and one that's called after navigation. Rich had suggested in https://github.com/sveltejs/sapper/issues/1278 introducing a method onNavigate would be called after navigation, so maybe the onNavigate here should be called onBeforeNavigate

[benmccann]

why do we need to check if the callback is already present?

[PatrickG]

Otherwise the callback could be added (and then later called) multiple times.

[PatrickG]

This could use some docs added under site/content/docs/ explaining the usage. That will also make the code easier to review. E.g. right now I'm not quite sure what the return value of onNavigate is used for

True.
onNavigate returns a function to unsubscribe like onMount.

I wonder if using the browser's beforeunload would be a better way to handle this than introducing canNavigate since it also covers the case where the user is closing the tab

canNavigate is an internal function (probably should be can_navigate) which calls the onNavigate callbacks.
It is not possible to prevent the navigation to another route with beforeunload. beforeunload could be used in addition to onNavigate.
The Problem with beforeunload is, that you can not show your own modal, you can not even change the text thats displayed. Also you can not return a promise, to asynchronous determine that the navigation should be prevented.

My other question is whether we can have a method that's called before navigation and one that's called after navigation. Rich had suggested in #1278 introducing a method onNavigate would be called after navigation, so maybe the onNavigate here should be called onBeforeNavigate

You are right, onBeforeNavigate makes more sense, I guess.

[benmccann]

Sorry this had gone unreviewed for so long. It looks like there's quite the merge conflict here. I don't think we'll be adding much functionality to Sapper at this point, but there is a ticket to add onNavigate to SvelteKit if you'd like to take a stab at it there: sveltejs/kit#560. Thanks so much for taking a stab at this!!