ref(sveltekit): Improve SvelteKit 2.0 Not Found server error handling #9901
Conversation
Lms24
changed the title
Lms24
force-pushed
the
lms/ref-sveltekit-improve-server-handleError
branch
from
87f2c17 to
8cfeb81
Compare
| // TODO: add backwards-compatible type for kit 1.x (soon) | ||
| type HandleClientErrorInput = Parameters<HandleClientError>[0]; |
There was a problem hiding this comment.
Will add this in a follow-up PR where I'll add filtering out "Not Found" errors on the client side
| @@ -34,7 +34,7 @@ describe('isHttpError', () => { | |||
| expect(isHttpError(httpErrorObject)).toBe(true); | |||
| }); | |||
|
|
|||
| it.each([new Error(), redirect(301, '/users/id'), 'string error', { status: 404 }, { body: 'Not found' }])( | |||
| it.each([new Error(), { status: 301, message: '/users/id' }, 'string error', { status: 404 }, { body: 'Not found' }])( | |||
There was a problem hiding this comment.
calling redirect throws since Sveltekit 2.0, so I rewrote this test to what redirect used to return in kit 1.x (this is not breaking or relevant for kit 2.0)
| const { error, event, status, message } = input; | ||
|
|
||
| // SvelteKit 2.0 offers a reliable way to check for a Not Found error: | ||
| if (status === 404 && message === 'Not Found') { |
There was a problem hiding this comment.
l: is it just fine to only check status code?
There was a problem hiding this comment.
I added the check for both, message and status because they're not determined identically:
So while I think it doesn't happen at the moment, a HttpError could be thrown with a 404 status code and another message. If we only check for 404, we'd also filter out that error - whatever it is. Are we fine with that?
There was a problem hiding this comment.
I would assume any 404 errors are not useful, but perhaps it's good to be conservative on what we filter for now.
There was a problem hiding this comment.
I would not report 404s as errors. Filtering by transaction/span status should be powerful enough for users to achieve what they want.
There was a problem hiding this comment.
I changed the check to only consider the 404 status. You're right - we shouldn't capture other 404s either.
| const { error, event, status, message } = input; | ||
|
|
||
| // SvelteKit 2.0 offers a reliable way to check for a Not Found error: | ||
| if (status === 404 && message === 'Not Found') { |
There was a problem hiding this comment.
I would not report 404s as errors. Filtering by transaction/span status should be powerful enough for users to achieve what they want.
Lms24
force-pushed
the
lms/ref-sveltekit-improve-server-handleError
branch
from
a4e6a66 to
51788bc
Compare
This PR improves our logic to filter out "Not Found" errors in our server-side
handleErrorhook wrapper:statusand) to check for "Not Found" errorsmessagetwo propertiesproperty don't exist.Not really happy with the two
@ts-expect-errorbut I didn't find anything that looks nicer and still guarantees type safety when we work with theinputobject. Definitely open to ideas!ref #9851