[fix] beforeNavigate external link

[dummdidumm]

Call beforeNavigate once with type unload on external navigation

Fixes #6726 Fixes #6770

Given the reasons in https://github.com/sveltejs/kit/issues/6726#issuecomment-1247106719 I think this is the most sensible thing to do

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• [x] This message body should clearly illustrate what problems it solves.
• [x] Ideally, include a test that fails without this PR but passes with it.

Tests

• [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 8399e639848b1f458dd63378a710be1979aa70b5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Rich-Harris]

Aside from https://github.com/sveltejs/kit/pull/6813#pullrequestreview-1108148449 this looks fine, though I can't shake the feeling that the code is trying to tell us we're thinking about some of this stuff in slightly the wrong way (and I don't mean this PR, I mean the code in general, this PR is just resurfacing it). E.g. things would probably be simpler if update had to be passed a NavigationIntent, and the logic for handling the SPA 404 case lived in goto or in start.js, since that's the only time it can happen.

Returning NavigationIntent | false | undefined from get_navigation_intent seems like it will confuse future us. I don't know that we need to distinguish between URLs that didn't match because they're external and URLs that didn't match because they didn't match — if we can treat them the same way we'll thank ourselves one day.

[dummdidumm]

I tried to refactor the code around update/navigate but failed to come up with something better than the current state. Passing a navigation_result instead of a intent to update looks promising at first, but we can't be sure that invalidate won't need the same checks that navigate needs (theoretically there could be a redirect triggered by the invalidation, etc).

Does this all hint that the types are not set correctly semantically? Should type: "unload" only be fired when the user closes the browser tab by hand and not during a navigation? Should a navigation to something external be type: "link"/"goto" and external can be determined by looking at to? params/routeId is null in this case, although that's also the case for 404, but that could be distinguished by looking at the url which would have a different host. In #6726 something similar with an additional unload boolean was proposed to make this easier - we could add such a property to the to type to make it easier to distinguish. to: { params, routeId, url, external }

[Rich-Harris]

Should type: "unload" only be fired when the user closes the browser tab by hand and not during a navigation?

Ya know... that makes a lot of sense. I'm struggling to think of any reason not to do that. I think to.url.origin !== location.origin is easy enough to do that we don't need to add an additional external property

[aradalvand]

That would mean that if you need to limit your handler to client-side navigations only (e.g. to show a loader), you'd have to do this:

beforeNavigate(({ type, to }) => {
    if (type == 'unload' || to.url.origin != location.origin) return;
});

Both of these conditions are necessary and neither one would be sufficient alone.

Far too verbose and complicated in my opinion, which is why I suggested unload as a separate boolean property, allowing you to simply do:

beforeNavigate(({ unload }) => {
    if (unload) return;
});

Why would this not be feasible? It is the superior approach as far as I can tell, with much less ambiguity.

---

The user manually closes the tab or types something else in the search bar and hits enter:

• type: manual (or native) (bikesheddable)
• unload: true
• to: null (basically means unknown)

The user clicks on an external link:

• type: link
• unload: true
• to: https://google.com/...

The user clicks on an internal 404 link or one that has the data-sveltekit-reload attribute set:

• type: link
• unload: true
• to: https://same-site.com/...

@Rich-Harris @dummdidumm Would like to know your thoughts on this. Thanks.

[dummdidumm]

Right now external and "reload this" links don't have a to property, it's null (because unload is triggered, which we would need to change). Internal links that aren't found have a to property with params and routeId being null.

• If we say that any links that cause a full page reload have their to property set to null, the distinction doesn't need two checks, you could just do if (to?.url.origin != location.origin and adding another boolean would just introduce two ways to do the same.
• If we say that having the url set for full page reload links, too, then we would need an additional property because we can't distinguish whether or not the event will cause a full page reload: type: "link", to: { params: null, routeId: null, url: "https://same-origin-you-are-currently-on.com/.. }

[dummdidumm]

I adjusted the code not call "unload" during navigation. The way to check if a client-side navigation is happening is to check for to?.routeId != null

[aradalvand]

That's certainly better, but to?.routeId != null is still somewhat unidiomatic. Wouldn't you agree? I don't think it'll be clear to someone new to the framework that that's how they're supposed to handle client-side navigations, unless they know in detail how these events work or unless this is mentioned explicitly in the documentation, both of which are signs of a slightly awkward API. But correct me if I'm wrong.

[dummdidumm]

While looking into this I came across this doc which discourages using the beforeunload event at all, as it could prevent the site from being eligible for bfcache. I'm now wondering if we should ditch the beforeNavigate behavior for unload events entirely (the scroll behavior thing could go into pagehide or sth similar instead). It would make sense because closing a tab isn't a navigation, but it might also miss some edge cases where people do location = 'somewhereelse' - is there a way to check this differently?

Either way, if we did remove the beforeNavigate call in unload (pro-con see last sentence above), it would make implementing this logic easier, when taking into account #6496 (update: proposing a different approach for that PR)

[PatrickG]

ditch the beforeNavigate behavior for unload events entirely

My guess is that most people that want to cancel() a navigation in beforeNavigation also want/need the current behavior of preventing the navigation to an external link/closing the tab. They would need to reimplement that in userland.

If the bfcache thing is a real concern that sveltekit wants to avoid, it is possible to add the beforeunload listener only while a beforeNavigation hook is active and handle the scrolling stuff in a pagehide listener. That way, only pages that use the beforeNavigation hook have the bfcache problem.

[dummdidumm]

Revisiting this:

The beforeUnload stuff is a thing for another PR (if at all)

Having a dedicated unload property that says "this will not be a SPA-style client-side navigation": I'm torn. It could be valuable to have routeId and params set for an internal link app but with data-sveltekit-reload on it; currently they are both null. I also have to admit that determining it through routeId being null is not idiomatic at first (but one could also argue that's SvelteKit language you'll get used to, which also used in other places; for example a 404 is also visible through routeId being null).

Either way, if we think the data-sveltekit-reload case is worthwhile, we need another property, at which point I'm split on whether it makes sense to have unload as a top level property besides to, type and from.

• Argument in favor: easier to use: beforeNavigate({ unload }) => { if (!unload) { // handle only client-side navs here.. vs beforeNavigate({ to }) => { if (!to || to.unload) {..
• Argument against: unload and type: 'unload' are somewhat duplicative

... which brings me to questioning my initial thought of using type: 'link' even if it's an external link or full-reload-link. One of the arguments of it was "the code is unreasonably hard writing it with type: 'unload', which hints at a design flaw", which is no longer the case. This line https://github.com/sveltejs/kit/blob/9a8c95544c831edb3c5c103e5228b751f86d7cc5/packages/kit/src/runtime/client/client.js#L1343 just needs to be changed to type: 'unload'. Maybe the previous design was right after all?

[aradalvand]

Argument against: unload and type: 'unload' are somewhat duplicative

I don't get this. If we have a separate unload flag then there wouldn't be a type of unload, type's values would then only be things like link or goto or whatever; representing the "type" of the navigation in the sense of what triggered it.

The problem with having unload as a type is that conceptually it doesn't really belong in the same category as things as link, goto, etc. The latter indicate "what triggered the navigation", whereas the former (unload) informs us about the "way" the navigation is going to take place. It's really answering a different question, if you will.

[dummdidumm]

We have type: 'unload' for the case where a user closes the browser window - so, no navigation, just unload. So we can't remove that type, because it's its own category/case.

[aradalvand]

@dummdidumm

We have type: 'unload' for the case where a user closes the browser window - so, no navigation, just unload. So we can't remove that type, because it's its own category/case.

Well, just don't call that type unload then. Obviously, it shouldn't be called that if have a separate property with the same name as that would create confusion. Call it something else; like manual for instance.

It would be type: manual or something — and also unload: true of course — in scenarios like that.

I had previously talked about this here.

[dummdidumm]

The name of the type is not the point, the point is that if it's type: 'unload' then a possible unload boolean is duplicate information in that case. unload can also be true for other types, but for type: 'unload', it will always be true

[aradalvand]

Okay, but I don't see how that's a problem. The two values just happen to overlap in one case. This sort of thing happens everywhere.

[dummdidumm]

After discussing this we decided to keep the types as-is and add unload as a top-level boolean. @Rich-Harris this is ready to review.

[aradalvand]

Nice to hear, thank you. But wouldn't the value unload for type be confusing when there's a boolean flag with the same name, as we discussed? Are you keeping it just to avoid a breaking change? Or is there some other reason?

[dummdidumm]

As explained above, type: 'unload' is different kind of navigation, it's called when for example you close the browser. Having this part of beforeNavigate is nice for things like "you have unsaved changes"-dialogs.

[aradalvand]

I'm not arguing for its removal, just changing the word unload to something else to avoid unnecessary confusion (while keeping the same underlying semantics). I talked about the same thing in this comment.

[Rich-Harris]

manual isn't a great alternative though. Almost all navigation is the result of some manual action!

One option we considered is close, which is appropriate in the case that it's being called as the result of someone closing a tab or the whole browser. It makes less sense in the case where someone is traversing the history stack across a document boundary.

It's definitely bikesheddable, but so far I'm not sure there's a better option than type === 'unload'. All suggestions welcome

[aradalvand]

manual isn't a great alternative though. Almost all navigation is the result of some manual action!

Fair enough!

One option we considered is close, which is appropriate in the case that it's being called as the result of someone closing a tab or the whole browser. It makes less sense in the case where someone is traversing the history stack across a document boundary. It's definitely bikesheddable, but so far I'm not sure there's a better option than type === 'unload'. All suggestions welcome

How about leave? It's more generic in terms of meaning compared to close and that makes it more fitting I think, makes more sense than close for that scenario you mentioned, for instance. And its counterpart for afterNavigate could be enter.

Another option is browser (or native), although admittedly somewhat vague, the advantage it has is that the same word can be used for both beforeNavigate and afterNavigate.

I like leave/enter best personally; the enter/leave naming can also be found in a lot of places like the mouseenter and mouseleave events. Tell me what you think. Though I still agree that too much bikeshedding here should probably be avoided.

[Rich-Harris]

Yeah, enter/leave is a good suggestion. I like that. In a similar vein start/end could also work, though enter arguably makes more sense than start if afterNavigate is fired after you click the back button from a different document.

One more piece of bikeshedding: should the boolean be unload or willUnload? navigation.willUnload feels slightly nicer to me than navigation.unload, now that I think about it.

[dummdidumm]

I like both suggestions, updated the PR

[aradalvand]

Yeah, enter/leave is a good suggestion. I like that. In a similar vein start/end could also work, though enter arguably makes more sense than start if afterNavigate is fired after you click the back button from a different document.

I'm fine with both, though enter/leave sounds somewhat more intuitive to me.

One more piece of bikeshedding: should the boolean be unload or willUnload? navigation.willUnload feels slightly nicer to me than navigation.unload, now that I think about it.

If this one's called willUnload then will its counterpart in afterNavigate be called hasLoaded or something?

[aradalvand]

Actually you know what, taking a step back, wouldn't you consider renaming the type property to cause or trigger? I feel like those names much more accurately describe what that property will actually represent from now on, type feels too generic actually. And then have a single "cause" value called browser or native or something for indicating navigations that are caused by some native browser action (closing the tab, etc.)

Because when you think about it more carefully, it fundamentally seems redundant to have 2 values (enter and leave) to describe the same kind of thing. You know. This doesn't happen for any other kind of type.

And then also maybe have the unload/willUnload property be called csr (client-side routing) or whatever for both beforeNavigate and afterNavigate, this will all unify things beautifully I reckon, and seems to be the least confusing, clearest design possible.

Please consider this. Let me know if I'm missing something.

[dummdidumm]

• willUnload only makes sense in the scope of beforeNavigate and $navigating, ideally the types would represent that and willUnload is not part of afterNavigate. csr sounds appealing at first because of the option that has the same name, but is closing the tab which triggers type: 'leave' some routing? I mean, it's not, so it's false, but the naming somewhat suggests that the opposite would be a full page reload navigation. willUnload therefore describes better to me that your app state is about to be destroyed
• having hasLoaded doesn't make sense to me, it would always be true, so this property has no meaning. The "ok app started up" is part of afterNavigate with type enter.
• I like trigger, but I'm not sure if it's that much better than type to justify this kind of breaking change. On the other hand, it could make the other renames more clear, since it's easy to detect that.
• enter vs leave and unifying them: enter is only available for afterNavigate and leave is only available for beforeNavigate. I think semantically they much more describe what is about to happen, so I feel like we should keep them. But the types could probably be tweaked.

[Rich-Harris]

I'm in favour of the current changes — given that enter only happens with afterNavigate and leave only happens with beforeNavigate, I'd argue it's much less confusing than having the same thing in both cases. It can be made into a type error that would guard against this sort of sleep-deprivation-induced code, for example:

beforeNavigate((navigation) => {
  if (navigation.type === 'enter') {
    // this doesn't make any sense!
  }
});

afterNavigate((navigation) => {
  if (navigation.type === 'leave') {
    // nor does this
  }
});

If the type was browser instead I can definitely imagine myself writing something like that and being confused about why it didn't work, since my brain equates 'before' and 'after' with 'earlier' and 'later', whereas to capture the start and end of the session you need to start with 'after' and end with 'before'.

Agree with @dummdidumm about hasLoaded. The point of having a separate willUnload boolean is that it's orthogonal to the navigation type, whereas an equivalent-but-opposite boolean only applies to a single navigation type, enter.

[Rich-Harris]

The types stuff would be a nice bonus but I'm not 100% sure what the best way to do that is, so I'll merge this and open a fresh issue