kit

kit copied to clipboard

Describe the bug

If you click on a link to an external URL on the page, or if you do a goto to an external URL, two successive beforeNavigate events will fire, the first one will have a type of link/goto, and the second one unload:

This behavior is strange, to say the least. First of all, beforeNavigate firing twice for the same navigation action is... well, awkward and unexpected. And if you have a navigation loader that you want to show only during client-side routing, you can't simply do this:

beforeNavigation(({ type } => {
    if (type == 'unload') return;
    // Show loader
});

Because then the loader would also appear for external, native browser navigations. The behavior is inconsistent with the "promise" @Rich-Harris made here:

Yep, type === 'unload' and !client would be equivalent. All other types would be client-side navigations

(emphasis mine)

This is now not the case, an instance of beforeNavigate firing with a type of something other than unload (like link in the example I provided above) COULD be a case of non-client-side navigation.

Instead, the API should've probably been designed such that we had unload as a separate boolean flag, alongside the type property, and then the event would only fire once when unloading would happen:

beforeNavigate({ unload, type } => {
    if (unload) return; // e.g. If the user clicked on an external link, here `unload` is `true` and `type` is `link`.
    // Show loader
});

Please feel free to let me know if I'm missing something. Thank you.

Reproduction

Explained above

Logs

No response

System Info

System:
    OS: Linux 5.15 Ubuntu 20.04.4 LTS (Focal Fossa)
    CPU: (4) x64 Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz
    Memory: 461.70 MB / 3.77 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 16.15.1 - /usr/bin/node
    Yarn: 1.22.15 - /usr/bin/yarn
    npm: 8.11.0 - /usr/bin/npm
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.64 
    @sveltejs/adapter-node: ^1.0.0-next.86 => 1.0.0-next.86 
    @sveltejs/kit: next => 1.0.0-next.480 
    svelte: ^3.44.0 => 3.49.0 
    vite: ^3.1.0-beta.1 => 3.1.0

Severity

serious, but i can work around it

Additional Information

No response

Could you please check this out so that hopefully this is fixed before 1.0? Thanks.

Gave this a stab today but it's not that easy conceptually

• what if an internal link is clicked but it has the data-sveltekit-reload attribute?
• what if an internal link is clicked that's a 404 (or some other reason for a full page reload), causing a full page reload?

In both these cases the navigation has to do its first steps in order to determine that it shouldn't do a client side navigation. Question is if we defer calling beforeNavigate and updating $navigating until after that, or if we treat these as internal navigations and don't call beforeNavigate in beforeunload again (the latter is probably my preference).

@dummdidumm

what if an internal link is clicked but it has the data-sveltekit-reload attribute? what if an internal link is clicked that's a 404 (or some other reason for a full page reload), causing a full page reload?

Well, in both of those cases a beforeNavigate with unload = true and type = link and origin = [site-url] will be fired. What would be wrong with that?!

In both these cases the navigation has to do its first steps in order to determine that it shouldn't do a client side navigation. Question is if we defer calling beforeNavigate and updating $navigating until after that, or if we treat these as internal navigations and don't call beforeNavigate in beforeunload again (the latter is probably my preference).

What I personally prefer is actually for beforeNavigate and afterNavigate to just be specific to client-side navigation, and if anyone wants to also listen for load and unload they could very easily do so by registering event handlers for the said events on window. I never really liked the idea of beforeNavigate and afterNavigate also firing for native browser navigations — and I have voiced this in the past — when there's already built-in events for that. Currently, in most cases you just have to do an annoying if (type == 'unload') return; or whatever in your handlers all the time.

But if that's a non-starter for you, then at the very least this all should be refined before 1.0. I think the approach I proposed (that is, having unload/load as separate boolean flags alongside type) is more sensible than the current one.

Let me know your thoughts.