-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix: Don't crash on hydratable serialization failures
#17315
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'svelte': patch | ||
| --- | ||
|
|
||
| fix: Don't crash on hydratable serialization failure | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,32 +57,28 @@ function encode(key, value, unresolved) { | |
|
|
||
| entry.serialized = devalue.uneval(entry.value, (value, uneval) => { | ||
| if (is_promise(value)) { | ||
| // we serialize promises as `"${i}"`, because it's impossible for that string | ||
| // to occur 'naturally' (since the quote marks would have to be escaped) | ||
| // this placeholder is returned synchronously from `uneval`, which includes it in the | ||
| // serialized string. Later (at least one microtask from now), when `p.then` runs, it'll | ||
| // be replaced. | ||
| const placeholder = `"${uid++}"`; | ||
| const p = value | ||
| .then((v) => `r(${uneval(v)})`) | ||
| .then((v) => { | ||
| entry.serialized = entry.serialized.replace(placeholder, `r(${uneval(v)})`); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }) | ||
| .catch((devalue_error) => | ||
| e.hydratable_serialization_failed( | ||
| key, | ||
| serialization_stack(entry.stack, devalue_error?.stack) | ||
| ) | ||
| ); | ||
|
|
||
| // prevent unhandled rejections from crashing the server | ||
| p.catch(() => {}); | ||
|
|
||
| // track which promises are still resolving when render is complete | ||
| unresolved?.set(p, key); | ||
| p.finally(() => unresolved?.delete(p)); | ||
|
|
||
| // we serialize promises as `"${i}"`, because it's impossible for that string | ||
| // to occur 'naturally' (since the quote marks would have to be escaped) | ||
| const placeholder = `"${uid++}"`; | ||
|
|
||
| (entry.promises ??= []).push( | ||
| p.then((s) => { | ||
| entry.serialized = entry.serialized.replace(placeholder, s); | ||
| }) | ||
| ); | ||
| // prevent unhandled rejections from crashing the server, track which promises are still resolving when render is complete | ||
| p.catch(() => {}).finally(() => unresolved?.delete(p)); | ||
|
|
||
| (entry.promises ??= []).push(p); | ||
| return placeholder; | ||
| } | ||
| }); | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import { test } from '../../test'; | ||
|
|
||
| export default test({ | ||
| mode: ['async'], | ||
| error: 'hydratable_serialization_failed' | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <script lang="ts"> | ||
| import { hydratable } from 'svelte'; | ||
| hydratable('key', () => new Promise(() => { throw new Error('nope') })); | ||
| </script> |
Uh oh!
There was an error while loading. Please reload this page.