From 9489a33caf6949d7cd9aad1dc158c288493b1e2b Mon Sep 17 00:00:00 2001 From: rixo Date: Fri, 31 May 2019 01:42:35 +0200 Subject: [PATCH 1/4] failing test for #710 --- test/apps/errors/src/routes/index.svelte | 3 ++- .../errors/src/routes/preload-reject.svelte | 7 ++++++ test/apps/errors/test.ts | 22 +++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 test/apps/errors/src/routes/preload-reject.svelte diff --git a/test/apps/errors/src/routes/index.svelte b/test/apps/errors/src/routes/index.svelte index a6b7896db..5ccc389a2 100644 --- a/test/apps/errors/src/routes/index.svelte +++ b/test/apps/errors/src/routes/index.svelte @@ -2,4 +2,5 @@ nope blog/nope -throw \ No newline at end of file +throw +preload-reject \ No newline at end of file diff --git a/test/apps/errors/src/routes/preload-reject.svelte b/test/apps/errors/src/routes/preload-reject.svelte new file mode 100644 index 000000000..e953df8fd --- /dev/null +++ b/test/apps/errors/src/routes/preload-reject.svelte @@ -0,0 +1,7 @@ + + +

Test has failed

\ No newline at end of file diff --git a/test/apps/errors/test.ts b/test/apps/errors/test.ts index 181c59189..29aa49f36 100644 --- a/test/apps/errors/test.ts +++ b/test/apps/errors/test.ts @@ -85,6 +85,28 @@ describe('errors', function() { ); }); + // https://github.com/sveltejs/sapper/issues/710 + // + // In reported issue, error elements disappear, which is not the case in + // our test case. This is because the default layout `` does not + // try to reclaim anything. Reporter probably has a custom `_layout.svelte` + // with some divs etc., and those will clear the existing DOM (if they + // don't find their element?). + // + // Here we're not testing that the error is still here, but that the page + // component has not been rendered (since only error page should be rendered + // here). So we don't need a custom layout. + // + it('does not replace server side rendered error', async () => { + await r.load('/preload-reject'); + await r.sapper.start(); + + assert.equal( + await r.text('h1'), + '500' + ); + }); + it('does not serve error page for explicit non-page errors', async () => { await r.load('/nope.json'); From 7559d39f0ed10789897ca414a3da0cbc25b636d1 Mon Sep 17 00:00:00 2001 From: rixo Date: Fri, 31 May 2019 01:45:19 +0200 Subject: [PATCH 2/4] prevent incorrect client rerender over ssr error (fixes #710) --- .../src/server/middleware/get_page_handler.ts | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/runtime/src/server/middleware/get_page_handler.ts b/runtime/src/server/middleware/get_page_handler.ts index 40e6ef8fa..864fde9f9 100644 --- a/runtime/src/server/middleware/get_page_handler.ts +++ b/runtime/src/server/middleware/get_page_handler.ts @@ -261,7 +261,7 @@ export function get_page_handler( session: session && try_serialize(session, err => { throw new Error(`Failed to serialize session data: ${err.message}`); }), - error: error && try_serialize(props.error) + error: error && serialize_error(props.error) }; let script = `__SAPPER__={${[ @@ -366,6 +366,25 @@ function try_serialize(data: any, fail?: (err) => void) { } } +// Try to serialize the given error, and ensure to return something truthy. +// Otherwise, the client will think that it got preloaded data with no error +// and rerender the page component over the error. +// +// See: https://github.com/sveltejs/sapper/issues/710 +// +function serialize_error(error: Error|{message: string}) { + if (!error) return null + let serialized = try_serialize(error) + if (!serialized) { + const {name, message, stack} = error as Error + serialized = try_serialize({name, message, stack}) + } + if (!serialized) { + serialized = '{}' + } + return serialized +} + function escape_html(html: string) { const chars: Record = { '"' : 'quot', From 20dd6d5147083989c0fd80b185c85e3b2d9da578 Mon Sep 17 00:00:00 2001 From: rixo Date: Sun, 31 May 2020 23:09:59 +0200 Subject: [PATCH 3/4] Update test/apps/errors/src/routes/preload-reject.svelte Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> --- test/apps/errors/src/routes/preload-reject.svelte | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/apps/errors/src/routes/preload-reject.svelte b/test/apps/errors/src/routes/preload-reject.svelte index e953df8fd..12a2e170d 100644 --- a/test/apps/errors/src/routes/preload-reject.svelte +++ b/test/apps/errors/src/routes/preload-reject.svelte @@ -1,7 +1,7 @@ -

Test has failed

\ No newline at end of file +

Test has failed

From ef1af06015a3efe6e78409ba4251388db01f11ec Mon Sep 17 00:00:00 2001 From: Conduitry Date: Mon, 1 Jun 2020 10:13:33 -0400 Subject: [PATCH 4/4] tidy --- .../src/server/middleware/get_page_handler.ts | 21 +++++++------------ test/apps/errors/test.ts | 12 ----------- 2 files changed, 8 insertions(+), 25 deletions(-) diff --git a/runtime/src/server/middleware/get_page_handler.ts b/runtime/src/server/middleware/get_page_handler.ts index 864fde9f9..6a4d4f519 100644 --- a/runtime/src/server/middleware/get_page_handler.ts +++ b/runtime/src/server/middleware/get_page_handler.ts @@ -366,23 +366,18 @@ function try_serialize(data: any, fail?: (err) => void) { } } -// Try to serialize the given error, and ensure to return something truthy. -// Otherwise, the client will think that it got preloaded data with no error -// and rerender the page component over the error. -// -// See: https://github.com/sveltejs/sapper/issues/710 -// -function serialize_error(error: Error|{message: string}) { - if (!error) return null - let serialized = try_serialize(error) +// Ensure we return something truthy so the client will not re-render the page over the error +function serialize_error(error: Error | { message: string }) { + if (!error) return null; + let serialized = try_serialize(error); if (!serialized) { - const {name, message, stack} = error as Error - serialized = try_serialize({name, message, stack}) + const { name, message, stack } = error as Error; + serialized = try_serialize({ name, message, stack }); } if (!serialized) { - serialized = '{}' + serialized = '{}'; } - return serialized + return serialized; } function escape_html(html: string) { diff --git a/test/apps/errors/test.ts b/test/apps/errors/test.ts index 29aa49f36..f54c15cbf 100644 --- a/test/apps/errors/test.ts +++ b/test/apps/errors/test.ts @@ -85,18 +85,6 @@ describe('errors', function() { ); }); - // https://github.com/sveltejs/sapper/issues/710 - // - // In reported issue, error elements disappear, which is not the case in - // our test case. This is because the default layout `` does not - // try to reclaim anything. Reporter probably has a custom `_layout.svelte` - // with some divs etc., and those will clear the existing DOM (if they - // don't find their element?). - // - // Here we're not testing that the error is still here, but that the page - // component has not been rendered (since only error page should be rendered - // here). So we don't need a custom layout. - // it('does not replace server side rendered error', async () => { await r.load('/preload-reject'); await r.sapper.start();