-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: await for settled instead of tick in navigate
#14800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 91ee377 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 |
|
In my app, I resorted to using an After seeing this PR, I tried importing Edit: |
|
Yeah, just awaiting settled in let resolver: (() => void) | null = null;
$effect.pre(() => {
void page.url;
resolver?.();
resolver = null;
});
onNavigate(({ complete }) => {
const promise = new Promise<void>((res) => {
resolver = res;
});
return new Promise((res) => {
document.startViewTransition(async () => {
res();
await complete;
await promise;
});
});
});Which is better than awaiting a random 100ms (which will also break if the promise in the new page take longer than that). |
|
I've tried to use the build from this PR directly and it worked great, but now after updating to v2.48.0, the issue appears again – |
|
Mmm I see |
|
You are right could be on the svelte side tho 🤔 |
| await tick(); | ||
| // svelte.settled is only available in Svelte 5 | ||
| if (/** @type {any} */ (svelte).settled) { | ||
| promises.push(/** @type {any} */ (svelte).settled()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can svelte.settled() take a long time? After upgrading to v.2.48.0 we suddenly start seeing full-page navigation where we previously saw client-side navigation. It seems like the e.preventDefault() for links is not triggered, so we often end up getting the native full-page navigation instead of sveltekit's client-side navigation.
If i go into our app and wait a little bit, then client-side navigation usually works. But if i go into our app and click on a link within <5-10s of initial load then a full page navigation is triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, i don't know if we're using svelte.settled - where is this defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you also upgrade svelte to the latest version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we have a different issue with the latest Svelte versions (since 5.41.1, 5.41.0 is the latest working version for us). We're heavily affected by this performance regression: sveltejs/svelte#16990
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the problem, unfortunately...previous svelte versions don't resolve settled 😔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, okay. We're kinda in a deadlock now, we can't upgrade Svelte or Sveltekit with the latest updates :/ With all of these being minor updates it feels kinda scary that there are regressions in performance and behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @MathiasWP the full-page navigations don't sound normal. Hydration shouldn't be taking that long. Is it possible for you to reproduce this and create an issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was because we used version 5.41.0 of Svelte together with the latest version of SvelteKit that had this PR merged. The bug disappeared in sveltekit after updating svelte with the fix in #17051.
It would be nice in the future to get a heads up if a sveltekit update requires a minimum version of Svelte, this wasn't really communicated anywhere :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't intentional. We're trying our best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minimum version of Svelte should always be the one specified in the peerDependency list of SvelteKit's package.json file. I think the package manager will tell you if it's not within the specified version range.
Closes #14711
This all sprouted from the fact that using
onNavigateto trigger view transitions doesn't work if the receiving component usesawait. The issue is that the DOM is not mounted yet when thecompletepromise resolves because it's still awaiting.But when trying to fix this bug, I realized that this was probably messing with focus management and scroll restoration because we were awaiting
tickinstead ofsettled. This changes that (using the namespaced import so that it also works for Svelte 4/Svelte 5 before settled versions).There doesn't seem to be an app that uses☺️
async: truein tests so I'm not sure how to test this (we should probably add one). Let me know if I should add onePlease don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
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