fix: Don't crash on hydratable serialization failures
#17315
Conversation
🦋 Changeset detectedLatest commit: ac41805 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 |
|
There was a problem hiding this comment.
I did verify that this crashes the test process prior to this PR -- it ends up with that super unhelpful "some test somewhere had a rejected promise" error. So we're all good now, for sure, and this will catch regressions.
| const p = value | ||
| .then((v) => `r(${uneval(v)})`) | ||
| .then((v) => { | ||
| entry.serialized = entry.serialized.replace(placeholder, `r(${uneval(v)})`); |
There was a problem hiding this comment.
As always all this recursive promise stuff is brain-bending, but I think we just had indirection here in the first place. There's no reason we need to actually return the string from this promise, we can just replace the placeholders inline.
kylejrp
left a comment
kylejrp
left a comment
There was a problem hiding this comment.
Looks great to me! Fixes the problem I was having entirely. The exception bubbles up to Fastify's global handler now. Thanks again for the quick fix! (And for correcting my faulty explanation haha 😅)
|
Thanks for the report! The bug was pretty difficult to figure out 😅 Promises are wild |
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
This one was fun to track down. Closes #17313.
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint