sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.49k stars 1.89k forks source link

A function to tell a sveltekit throwable redirect/error/invalid from a normal error. #7941

Closed mavdotjs closed 1 year ago

mavdotjs commented 1 year ago

Describe the problem

I'm always frustrated when I am trying to handle errors in a request that uses sveltekit throwables (redirect/error/invalid) and I have to write extra code to tell if its a sveltekit throwable or not, such as:

import { error } from '@sveltejs/kit'
import type { RequestHandler } from './$types'

export let POST: RequestHandler = async ({ locals, request }) => {
    try {
        const user = await locals.getUser()
        if(!user) throw error(401, "Must be logged in to create a thingymabob")
        const data = await request.json()
        if(!data) throw error(400, "Data must be JSON")
        const createdThingy = await DB.thingies.create({
            name: data.name
        })
        return json(createdThingy)
    } catch(e) {
        // See If the error should be ignored or sent to the client
    }
    const user = (await locals.getSession())?.userId || () => { throw error() }

}

Describe the proposed solution

My proposed solution is to add a function that checks if an error is actually an error or is a sveltekit throwable and should be re-thrown

import { error, isKitError } from '@sveltejs/kit'
import type { RequestHandler } from './$types'

export let POST: RequestHandler = async ({ locals, request }) => {
    try {
        const user = await locals.getUser()
        if(!user) throw error(401, "Must be logged in to create a thingymabob")
        const data = await request.json()
        if(!data) throw error(400, "Data must be JSON")
        const createdThingy = await DB.thingies.create({
            name: data.name
        })
        return json(createdThingy)
    } catch(e) {
        if(isKitError(e)) throw e // rethrow
        // handle DB errors normally
    }
    const user = (await locals.getSession())?.userId || () => { throw error() }

}

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

eltigerchino commented 1 year ago

The SvelteKit runtime code does it such as below:

if (error instanceof Redirect) {
   throw error;
}

So, you could create a helper function that checks if the error matches these types.

function isKitError(error) {
  return (
    error instanceof HttpError ||
    error instanceof Redirect ||
    error instanceof ValidationError
  );
}
mavdotjs commented 1 year ago

Solved.

xmlking commented 1 year ago

wonder how this is possible ? SvelteKit only export interface not classes!

image
eltigerchino commented 1 year ago

Good catch. I never actually tested this. I wonder if it's worth exporting these classes or the helper function itself for users. Maybe the issue should be re-opened / re-created.

Rich-Harris commented 1 year ago

The idea was that you wouldn't try-catch your request handlers — that's extremely annoying regardless of whether or not you can differentiate Kit from non-Kit errors. Instead of this...

import { error } from '@sveltejs/kit'
import type { RequestHandler } from './$types'

export let POST: RequestHandler = async ({ locals, request }) => {
    try {
        const user = await locals.getUser()
        if(!user) throw error(401, "Must be logged in to create a thingymabob")
        const data = await request.json()
        if(!data) throw error(400, "Data must be JSON")
        const createdThingy = await DB.thingies.create({
            name: data.name
        })
        return json(createdThingy)
    } catch(e) {
        // See If the error should be ignored or sent to the client
    }
    const user = (await locals.getSession())?.userId || () => { throw error() }

}

...you'd have this:

import { error } from '@sveltejs/kit'
import type { RequestHandler } from './$types'

export let POST: RequestHandler = async ({ locals, request }) => {
    const user = await locals.getUser()
    if(!user) throw error(401, "Must be logged in to create a thingymabob")
    const data = await request.json()
    if(!data) throw error(400, "Data must be JSON")
    const createdThingy = await DB.thingies.create({
        name: data.name
    })
    return json(createdThingy)
}
// src/hooks.server.js
export function handleError({ error, event }) {
  // DB errors get handled here
}

Does that fall short?

xmlking commented 1 year ago

@Rich-Harris yes, your solution is clean in actions / loaders / apis, but we might end up with many if/else statements to introspect error type and extract human friendly error message in global handleError function.

I feel keeping error handling close to source of error might be easy to maintain code. See for example below , I have to return error message in case of load, but return fail(400, {err}) in case of actions https://github.com/xmlking/svelte-starter-kit/blob/main/src/routes/(app)/dashboard/policies/%2Bpage.server.ts

https://github.com/xmlking/svelte-starter-kit/blob/main/src/routes/(app)/dashboard/policies/%5Bid%5D/%2Bpage.server.ts

Rich-Harris commented 1 year ago

we might end up with many if/else statements to introspect error type and extract human friendly error message in global handleError function

handleError isn't a place to put a lot of logic, it's a fallback — if you end up there, it's something you should treat as a bug that needs fixing. The idea is that Kit errors are the only errors that get thrown from load, and those that end up in handleError all get the same treatment (e.g. log the error, report 'Internal Error' to the user). In some cases that might mean having a thin layer around your database client that translates client errors into something human-readable/actionable, to avoid repetition across load functions

I have to return error message in case of load, but return fail(400, {err}) in case of actions

One is 'this broke', another is 'try again' — they're different beasts. It wouldn't make sense for them to have the same signature.

Rich-Harris commented 1 year ago

I'll close this for now as it looks like the motivating use cases for exposing these classes are better solved in other ways. Open to revisiting in future if necessary

mavdotjs commented 9 months ago

just found out this was added in (I haven't done a lot of front end recently, been working mostly on backend/realtime stuff and discord bots)