From e4d66f06200df9e86badacf3f6601bcb957c321a Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 21 Feb 2022 22:29:41 -0500 Subject: [PATCH 1/6] prevent double-fixing of stack traces - fixes #3638 --- .changeset/angry-pugs-play.md | 5 +++++ packages/kit/src/core/dev/plugin.js | 11 ++++++++++- .../apps/basics/src/routes/errors/endpoint.json.js | 1 - packages/kit/test/apps/basics/test/test.js | 2 +- 4 files changed, 16 insertions(+), 3 deletions(-) create mode 100644 .changeset/angry-pugs-play.md diff --git a/.changeset/angry-pugs-play.md b/.changeset/angry-pugs-play.md new file mode 100644 index 000000000000..323590c972ff --- /dev/null +++ b/.changeset/angry-pugs-play.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +Prevent double-fixing of error stack traces in dev mode diff --git a/packages/kit/src/core/dev/plugin.js b/packages/kit/src/core/dev/plugin.js index 578072c4c48c..969ad6c2299d 100644 --- a/packages/kit/src/core/dev/plugin.js +++ b/packages/kit/src/core/dev/plugin.js @@ -232,17 +232,26 @@ export async function create_plugin(config, cwd) { const template = load_template(cwd, config); + // we want to avoid double-fixing stack traces, since + // that will typically make everything much worse + const fixed_errors = new WeakSet(); + const rendered = await respond(request, { amp: config.kit.amp, csp: config.kit.csp, dev: true, floc: config.kit.floc, get_stack: (error) => { - vite.ssrFixStacktrace(error); + if (!fixed_errors.has(error)) { + vite.ssrFixStacktrace(error); + } + return error.stack; }, handle_error: (error, event) => { vite.ssrFixStacktrace(error); + fixed_errors.add(error); + hooks.handleError({ error, event, diff --git a/packages/kit/test/apps/basics/src/routes/errors/endpoint.json.js b/packages/kit/test/apps/basics/src/routes/errors/endpoint.json.js index c3c604c268ca..488a97526280 100644 --- a/packages/kit/test/apps/basics/src/routes/errors/endpoint.json.js +++ b/packages/kit/test/apps/basics/src/routes/errors/endpoint.json.js @@ -1,4 +1,3 @@ -/** @type {import('@sveltejs/kit').RequestHandler} */ export function get() { throw new Error('nope'); } diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index c83e450369fb..22ca461a5b52 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -915,7 +915,7 @@ test.describe.parallel('Errors', () => { ); const contents = await page.textContent('#stack'); - const location = 'endpoint-shadow.js:1:8'; // TODO this is the wrong location, but i'm not going to open the sourcemap can of worms just now + const location = 'endpoint-shadow.js:3:8'; if (process.env.DEV) { expect(contents).toMatch(location); From d98924fa01484476e7aa9415284bee46626787b3 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 22 Feb 2022 00:23:07 -0500 Subject: [PATCH 2/6] dont mutate errors --- packages/kit/src/core/dev/plugin.js | 39 +++++++++++++++++++---------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/packages/kit/src/core/dev/plugin.js b/packages/kit/src/core/dev/plugin.js index 969ad6c2299d..64e2b107e519 100644 --- a/packages/kit/src/core/dev/plugin.js +++ b/packages/kit/src/core/dev/plugin.js @@ -134,6 +134,22 @@ export async function create_plugin(config, cwd) { }; } + /** @param {Error} error */ + function fix_stack_trace(error) { + // ideally vite would expose ssrRewriteStacktrace, but + // in lieu of that, we can implement it ourselves. we + // don't want to mutate the error object, because + // the stack trace could be 'fixed' multiple times, + // and Vite will fix stack traces before we even + // see them if they occur during ssrLoadModule + const original = error.stack; + vite.ssrFixStacktrace(error); + const fixed = error.stack; + error.stack = original; + + return fixed; + } + update_manifest(); vite.watcher.on('add', update_manifest); @@ -232,28 +248,25 @@ export async function create_plugin(config, cwd) { const template = load_template(cwd, config); - // we want to avoid double-fixing stack traces, since - // that will typically make everything much worse - const fixed_errors = new WeakSet(); - const rendered = await respond(request, { amp: config.kit.amp, csp: config.kit.csp, dev: true, floc: config.kit.floc, get_stack: (error) => { - if (!fixed_errors.has(error)) { - vite.ssrFixStacktrace(error); - } - - return error.stack; + return fix_stack_trace(error); }, handle_error: (error, event) => { - vite.ssrFixStacktrace(error); - fixed_errors.add(error); - hooks.handleError({ - error, + error: new Proxy(error, { + get: (target, property, receiver) => { + if (property === 'stack') { + return fix_stack_trace(error); + } + + return Reflect.get(target, property, target); + } + }), event, // TODO remove for 1.0 From a4a544977495928677236ae73e63eab024a8847a Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 22 Feb 2022 00:26:53 -0500 Subject: [PATCH 3/6] failing test --- .../apps/basics/src/routes/errors/init-error-endpoint.js | 9 +++++++++ packages/kit/test/apps/basics/test/test.js | 9 ++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js diff --git a/packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js b/packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js new file mode 100644 index 000000000000..879375f1814f --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js @@ -0,0 +1,9 @@ +thisvariableisnotdefined; + +export function get() { + return { + body: { + answer: 42 + } + }; +} diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 22ca461a5b52..8e554da291dc 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -906,7 +906,7 @@ test.describe.parallel('Errors', () => { expect(lines[0]).toMatch('nope'); if (process.env.DEV) { - expect(lines[1]).toMatch('endpoint-shadow'); + expect(lines[1]).toMatch('endpoint-shadow.js:3:8'); } expect(res && res.status()).toBe(500); @@ -962,6 +962,13 @@ test.describe.parallel('Errors', () => { ); expect(await page.innerHTML('h1')).toBe('500'); }); + + test('error evaluating module', async ({ request }) => { + const response = await request.get('/errors/init-error-endpoint'); + + expect(response.status()).toBe(500); + expect(await response.text()).toMatch('thisvariableisnotdefined is not defined'); + }); }); test.describe.parallel('ETags', () => { From 9a6a21b8b0ba45ac782341d47a13580f45fec780 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 22 Feb 2022 00:32:59 -0500 Subject: [PATCH 4/6] linting etc --- packages/kit/src/core/dev/plugin.js | 2 +- .../test/apps/basics/src/routes/errors/init-error-endpoint.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/kit/src/core/dev/plugin.js b/packages/kit/src/core/dev/plugin.js index 64e2b107e519..80c1e54f3e17 100644 --- a/packages/kit/src/core/dev/plugin.js +++ b/packages/kit/src/core/dev/plugin.js @@ -259,7 +259,7 @@ export async function create_plugin(config, cwd) { handle_error: (error, event) => { hooks.handleError({ error: new Proxy(error, { - get: (target, property, receiver) => { + get: (target, property) => { if (property === 'stack') { return fix_stack_trace(error); } diff --git a/packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js b/packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js index 879375f1814f..bd8a8d6d6000 100644 --- a/packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js +++ b/packages/kit/test/apps/basics/src/routes/errors/init-error-endpoint.js @@ -1,4 +1,5 @@ -thisvariableisnotdefined; +// @ts-expect-error +thisvariableisnotdefined; // eslint-disable-line export function get() { return { From 239023446cf50e60e24359aac1f3149cf94fc401 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 22 Feb 2022 17:50:35 -0500 Subject: [PATCH 5/6] skip test for now --- packages/kit/test/apps/basics/test/test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 8e554da291dc..03e9318ea9c6 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -963,7 +963,8 @@ test.describe.parallel('Errors', () => { expect(await page.innerHTML('h1')).toBe('500'); }); - test('error evaluating module', async ({ request }) => { + // TODO re-enable this if https://github.com/vitejs/vite/issues/7046 is implemented + test.skip('error evaluating module', async ({ request }) => { const response = await request.get('/errors/init-error-endpoint'); expect(response.status()).toBe(500); From aa9e51b68e9c16172b5032f63a2c57c310069851 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 23 Feb 2022 15:15:34 -0500 Subject: [PATCH 6/6] add link to vite issue --- packages/kit/src/core/dev/plugin.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/kit/src/core/dev/plugin.js b/packages/kit/src/core/dev/plugin.js index 80c1e54f3e17..8e173b6cde65 100644 --- a/packages/kit/src/core/dev/plugin.js +++ b/packages/kit/src/core/dev/plugin.js @@ -136,6 +136,8 @@ export async function create_plugin(config, cwd) { /** @param {Error} error */ function fix_stack_trace(error) { + // TODO https://github.com/vitejs/vite/issues/7045 + // ideally vite would expose ssrRewriteStacktrace, but // in lieu of that, we can implement it ourselves. we // don't want to mutate the error object, because