Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/da3cc3dd97cccaa76aabec54cf66c65d964059e0Open in Note This PR is from a fork. A maintainer must approve approve each commit before it can be built and installed. |
🦋 Changeset detectedLatest commit: da3cc3d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| // `load_cache` no longer holds this fork, so nothing else will discard it | ||
| void load_cache_fork?.then((f) => f?.discard()); | ||
| nav.reject(new Error('navigation aborted')); | ||
| return; |
There was a problem hiding this comment.
run_on_navigate_callbacks registers functions returned from onNavigate before this check, and this return leaves them in after_navigate_callbacks. A function returned for an aborted navigation runs when the next navigation commits, although its page never rendered.
There was a problem hiding this comment.
Confirmed, the aborted navigation's returned function ran when the next one completed. run_on_navigate_callbacks() now returns those functions instead of registering them, and navigate() registers them after the token check, so an aborted navigation never registers its function. The shallow-routing caller registers them immediately, as before. The basics test now also asserts that only the committed navigation's function runs. It failed without this change, and passes with it.
…into fix-superseded-onnavigate-commit
…eded later, including shallow ones
…nnavigate-commit # Conflicts: # packages/kit/src/runtime/client/client.js
navigate()checksnavigation_tokenonce its route has loaded, then awaitsonNavigatecallbacks and commits without checking again. When a callback returns a promise (the view transition recipe from the docs holds the navigation until the transition's update callback runs), a newer navigation can start during that wait, and the superseded one still setscurrentand applies its render tree when its callbacks settle. This adds the same token check after theonNavigatecallbacks as the one after load.With
experimental.forkPreloads,navigate()has already taken the preload fork out ofload_cacheat that point, so nothing else can reach it. The new abort path therefore discards the fork itself, the same waydiscard_load_cache()does.Functions returned by
onNavigatemust also not outlive an aborted navigation, or they run when the next navigation completes.run_on_navigate_callbacks()used to register them before the check. It now returns them, and the caller registers them only if the navigation is still current.finish_navigation()also receives them and removes them when it aborts, which covers a navigation superseded while its render settles. They stay registered before the commit, so they still run in the same order relative to otherafterNavigatecallbacks. Each registration wraps the returned functions in entries of its own.after_navigate_callbacksis aSet, so if two navigations'onNavigatecalls returned the same function object, removing the aborted navigation's registration would otherwise remove the newer navigation's too.A shallow
goto()has the same gap inupdate_state(): superseded whileonNavigatewas pending, it still applied its page state and registered its functions. It now gets the same check. As with a full navigation aborted at that point, its history entry has already been pushed and stays; the check only stops the page update and the registration.On
version-3the newer navigation still commits afterwards, so the result is transient: the superseded page mounts, renders and runs its effects, and itsonNavigatereturn value is registered as anafterNavigatecallback, just before the newest navigation replaces it.On 2.x (2.70.3,
main) the same gap leaves the wrong page in place. There, a navigation only sends thedata_Nprops whose node data differs fromcurrentat load time. If a superseded navigation commits between a newer navigation's load and its commit, the newer navigation's props are diffed against a stalecurrent: it commits withoutdata_Nfor a page whose data didn't change relative to thatcurrent. The URL then shows the newest route while the page keeps the superseded route's data. We hit this with rapid link clicks and Back under view transitions. I'm happy to open a backport againstmainif that's wanted.No existing issue covers this. #12809 involved a pending
onNavigatetoo, but that was a different symptom.Test
navigation-lifecycle/on-navigate-superseded/[id]holds every navigation inonNavigate. The test starts a navigation tob, then a newer one back toa, releases the superseded one first, and asserts thatbnever renders and that only the committed navigation'sonNavigatereturn value runs. It fails onversion-3without the change (Received: "b"), fails without the registration change (the aborted navigation's function runs), and passes 20/20 in dev and build with both. A second case runs the same sequence with a shallowgoto()as the superseded navigation. Without the change, the superseded navigation's state is applied (Received: "active").In
async, which enablesforkPreloads,fork/supersededpreloads a page that subscribes to a store counting its subscribers. The navigation to that page is held inonNavigateand superseded, and the test asserts that releasing it drops the count back to 0, meaning the fork was discarded. Without the discard it fails (Received: 1), and it passes 20/20 in dev and build with it.Also in
async,navigation-settleholds a page's render with a top-levelawait, and a newer navigation starts before it settles. The test asserts that only the newer navigation'sonNavigatereturn value runs. Without removing them on that abort it fails (the held navigation's function runs), and it passes 20/20 in dev and build with it. The page also returns one shared function object from every navigation, and the test asserts that the newer navigation still runs it. Without the per-registration entries, the aborted navigation's cleanup removes it.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkRan
pnpm format,pnpm lint,pnpm -F @sveltejs/kit test:unit, andpnpm checkinpackages/kit,basicsandasync(all clean), plus thebasics,optionsandasyncPlaywright suites on Chromium in dev and build.options,asyncandbasicsbuild pass. Inbasicsdev, a fewLoadandSPA modetests fail under the full suite's load, and they did so before this change too. They pass when rerun on their own. I didn't run the fullpnpm test.Changesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits