RFC: Standardise ServerErrors

[shairez]

Discussed in #200

^{Originally posted by DustinJSilk December 15, 2024}

What is it about?

ServerErrors should not return a value on the client, they should be thrown, and the error types should be the same for routeLoaders

What's the motivation for this proposal?

Problems you are trying to solve:

• Throwing a ServerError in a server$ function allows us to change the status code and payload of the repsonse. On the client, the value of the error is returned, rather than having the client throw the error so that it can be caught. This brings about a few challenges:
  • This breaks JS norms. It is expected that throwing an error means the error should be caught, instead, it is magically caught and the value is returned by the server$ functions.
  • This means awkward types must be used as the server$ function has to return a tuple to handle the error path. See the original PR for an example: feat(server$): config argument, optional GET, ServerError qwik#6290
  • The user has to handle 2 types of errors now: error values and thrown errors such as transient errors. Thus a try/catch is still needed along with the error value response.
  • Updating a server$ function which previously might throw a standard error now also requires changes to how the function is called due to the change in it's signature with a tuple response
• A routeLoader$ throws an error using the syntax throw event.error(500, 'some data'). This has 2 significant issues:
  • If we are running middleware to correctly set http status codes, we cannot simply throw a ServerError as the routeLoader$ expects a different error instance to the server$.
  • Throwing an error is only possible when you have access to the event object, which makes implementing middleware more difficult.

Goals you are trying to achieve:

• Standardise the error classes across server functions.
• Handle server$ errors as if they are standard errors on the client.

Any other context or information you want to share:

This would be a breaking change, but I dont think this API is widely used currently considering it doesn't work in a dev build due to this error: QwikDev/qwik#7165
This means we might be able to implement this change as a minor version bump, although it would technically be a breaking change.

---

Proposed Solution / Feature

What do you propose?

• Standardise the server$ and routeLoader$ error types to both use a ServerError class which isn't part of the event object.
• Have the client throw ServerErrors rather than returning the error payload as if its a successful response

Code examples

const useData = routeLoader$(() => {
  throw ServerError(500, 'some data')
})

const getData = server$(() => {
  throw ServerError(500, 'some data')
})

export default component$(() => {
  useVisibleTask(() => {
    getData()
      .then()
      .catch(err => {
        // handle error
      }) 
  })
}) 

---

Links / References

Original ServerError PR: QwikDev/qwik#6290

[DustinJSilk]

Here's a PR tracking this work: QwikDev/qwik#7185

Some remaining tasks:

• Add tests to ensure @plugin middleware can intercept errors thrown in server$ and routeLoader$
• Remove ErrorResponse handling
• Replace the error() function on the event object to return a ServerError to maintain backwards compatibility
• Add a documentation page on error handling
  • Overview on the ServerError (generic type for serialised response data + custom status codes + how to throw and catch)
  • Adding structured error logs with a plugin
  • Hiding errors on production (intercept and return a generic error response)
  • Using the event.error() helper functions simply create a ServerError instance

This change would also remove the error log here. Since errors can be intercepted by middleware this should be left to the user. When errors arent intercepted, they're returned to the frontend by default. If this is difficult for new comers facing their first errors, we can add a logging plugin to the starter templates.

@shairez please let me know if Ive missed anything.

[DustinJSilk]

The PR now includes documentation and tests for error middleware

[shairez]

Thanks a lot @DustinJSilk ! 🙏
I DM'ed you, let's chat

[DustinJSilk]

A few questions for the community:

• How should we handle error logging for first time users? Should we include a error logging middleware in the starter templates?
• How should we handle returning serialised data in an error when it is primitive data like a string or enum? Originally, calling a roueLoader event.error() would return an html page with the error response.
  This is great when you need it, but what if you want to return an enum which is an error status code? If we always return primitive data as HTML, then you can't access the primitive data in the client. We have a few options:
  • Only return qwik-json if the ServerError is thrown with an object type which means you cant return primitive data outside of an html response
  • Continue to have an ErrorResponse and ServerError types, where the ServerError returns serialised data, and ErrorResponse returns HTML. Then the throw event.error(500, "") always returns the ErrorResponse. This would make error handling middleware heavier as errors arent standardised completely
  • Have an option in the ServerError / event.error() to toggle between HTML or data

@shairez

[shairez]

About the first point- @QwikDev/qwik-team WDYT?

About the second point - maybe we could add a middleware just for the API requests (onGet etc) that marks them as a "data error response" to avoid sending the default error html page (if Qwik server was used for its API at that case)

[shairez]

@DustinJSilk we talked with @mhevery and he suggested looking at the accept header and if it's test/html we can return the html error page.

WDYT?

[DustinJSilk]

Oh, of course! Thats a great idea and should be simple to implement 😄

[DustinJSilk]

Seems like server$ sends Accept: */* as it can return various types of data (json, text, html).
I've done a rudimentary test to check that it equals */* which seems to work but likely isnt a great solution.

I'll need to check what the routeLoaders and actions send, during SSR and CSR

[DustinJSilk]

In my tests, it looks like both routeLoader$ and server$ have the same Accept headers which makes this difficult.

During SSR: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/* [...]
And in CSR: */*

[shairez]

@DustinJSilk but can't we assume that route loaders and server$ would be called in the context of an html rendered app?

meaning, only calls that explicitly send that they accept json as a response should be getting the json / text error, and all the rest will receive the HTML error page.

Or else I'm missing something here

[DustinJSilk]

Hmm, possibly.

We would need to then update the client server$ fetch so that it adds the application/qwik-json, text/qwik-json-stream, application/json, text/plain accept header, which is currently */* which should be fine as I don't think we allow returning text/html from a server$.

A server$ can be called inside a routeLoader$ or a task$ which would have the text/html header, but you might try something like this:

routeLoader$(() => {
  try {
    await someServer$Fun()
  } catch (err) {

  }
})

Would the err in the catch be html or the original thrown error? I might need to test this otherwise

[DustinJSilk]

@shairez ive updated the PR to test for text/html and incuded further e2e tests for error handling

[DustinJSilk]

@shairez gentle nudge here, can we progress this to the next phase?

[shairez]

thanks Dustin!
DM'ed you