-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: handle remote query redirects #15222
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
fix: handle remote query redirects #15222
Conversation
🦋 Changeset detectedLatest commit: 45c99a9 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 |
…thub.com/Antonio-Bennett/kit into workaround/query-redirect-error-handling
|
This isn't really the correct solution here. The problem is that somewhere in Svelte or SvelteKit, when a promise is created and stashed, it's not having a So basically there's somewhere we're missing a |
|
Yeah def thought so as well. Just put this out for an immediate workaround for now. Will keep digging around to see if I can find where that catch isn't being added! :) |
Co-authored-by: Elliott Johnson <hello@ell.iott.dev>
|
@elliott-with-the-longest-name-on-github thanks for your feedback and review! Committed the change :) |
teemingc
left a comment
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.
Could we add a test for this?
|
@teemingc added, is it satisfactory? |
|
@teemingc Hey sorry don't want to be a bother, know you have a lot on your plate just wondering if you could take a look in case this slipped by |
|
So sorry about that! It definitely did slip by 🤦🏼 It's 1 AM here so I'll take a look at this tomorrow. Thanks! |
|
I was a little confused when the test still passed even when I commented out the fix. Turns out the console.log from the original issue is caused by us passing the redirect error to the I've cleaned it up a bit so that the test does fail if a |
|
@teemingc thanks for taking a look and bringing it across the finish line! 🐐 |
Summary
onrejectedhandler when calling the async SSR render thenable to mark the underlying render promise as handledRedirectthrown during render as a real redirect response inrender_pageContext
The async render path returns a thenable from Svelte’s server renderer. Calling
.thenwithout anonrejectedmeans the underlying async render promise can reject (e.g.Redirect) before any handler is attached, which triggers Node’s unhandled rejection detection. Passing anonrejectedhandler attaches a rejection handler directly to the underlying promise.Separately, when a
Redirectis thrown during render (e.g. via a remotequeryused in a component), it currently falls into the generic error handler and returns a 500. HandlingRedirectinrender_pageensures the response redirects instead.How This Relates To Svelte
The unhandled rejection itself originates in Svelte’s SSR runtime (
Renderer.run()does not attach a rejection handler or tie the async work torenderer.promise). The Svelte PR below addresses that root cause; this Kit PR handles the redirect response once the render error propagates.Notes
Related
querywithredirectproduce 500undefinederror #14883Test Plan
queryduring render no longer triggers unhandled rejection and now redirects properlyPlease 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