Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local Error example in docs does not work #966

Open
dtinth opened this issue Dec 23, 2024 · 10 comments
Open

Local Error example in docs does not work #966

dtinth opened this issue Dec 23, 2024 · 10 comments
Labels
bug Something isn't working

Comments

@dtinth
Copy link

dtinth commented Dec 23, 2024

What version of Elysia is running?

1.2.2

What platform is your computer?

Darwin 24.1.0 arm64 arm

What steps can reproduce the bug?

Run the example from https://elysiajs.com/essential/life-cycle.html#local-error

The if clause is removed to simulate an error case.

import { Elysia } from "elysia";

new Elysia()
  .get("/", () => "Hello", {
    beforeHandle({ error }) {
      return error(401);
    },
    error({ error }) {
      return "Handled";
    },
  })
  .listen(3000);

What is the expected behavior?

"Handled"

What do you see instead?

The error hook is not called, got "Unauthorized" instead.

Additional information

No response

Have you try removing the node_modules and bun.lockb and try again yet?

No response

@dtinth dtinth added the bug Something isn't working label Dec 23, 2024
@SaltyAom
Copy link
Member

The documentation is incorrect.

onError is designed to catch unexpected error which is thrown.

The code here is using return which is an intended HTTP error to return a client. So it's not technically an error (status should be a more suitable name for this).

The code should be using throw instead which will propagate and HTTP status as unexpected error which will be put into error

The documentation should be fixed with this commit.

However, it can be proposed that this behavior be changed in the future.

@dtinth
Copy link
Author

dtinth commented Dec 25, 2024

@SaltyAom Thanks for the explanation.

From trying it out, it seems that the error() method returns an ElysiaCustomStatusResponse which does not inherit from Error.

As a result, if I do this:

throw error("I'm a teapot", { message: "meow!" });

Without setting .onError(), I get this output:

HTTP/1.1 418 I'm a Teapot
content-type: text/plain;charset=utf-8
Date: Wed, 25 Dec 2024 09:38:44 GMT
Content-Length: 15

[object Object]

I expected a JSON-stringified response, but instead of that I get [object Object] as a response.

It seems that Elysia doesn’t have default handling when ElysiaCustomStatusResponse, so my question is that is it idiomatic to throw error(…)?

Update: I created a proposal to make this idiomatic:

@SaltyAom
Copy link
Member

@SaltyAom Thanks for the explanation.

From trying it out, it seems that the error() method returns an ElysiaCustomStatusResponse which does not inherit from Error.

As a result, if I do this:

throw error("I'm a teapot", { message: "meow!" });

Without setting .onError(), I get this output:

HTTP/1.1 418 I'm a Teapot
content-type: text/plain;charset=utf-8
Date: Wed, 25 Dec 2024 09:38:44 GMT
Content-Length: 15

[object Object]

I expected a JSON-stringified response, but instead of that I get [object Object] as a response.

It seems that Elysia doesn’t have default handling when ElysiaCustomStatusResponse, so my question is that is it idiomatic to throw error(…)?

Update: I created a proposal to make this idiomatic:

This is a bug which should have been fixed with b94dab2, published under 1.2.7

@SaltyAom
Copy link
Member

SaltyAom commented Dec 27, 2024

@dtinth starting from 1.2.7, onError can intercept Elysia.error which is thrown by specifying code as a number (ideally I would like to use named error as string but that would be a breaking change unfortunately)

Which mean this works:

new Elysia()
	.onError(({ error, code }) => {
		switch(code) {
			case 418:
				console.log(error)
		}
	})
	.get('/', () => {
		throw error("I'm a teapot")
	})
	.listen(3000)

While this is not:

new Elysia()
	.onError(({ error, code }) => {
		switch(code) {
			case 418:
				console.log(error)
		}
	})
	.get('/', () => {
		return error("I'm a teapot")
	})
	.listen(3000)

Alternatively, we can use StatusMap if prefer:

const app = new Elysia()
	.onError(({ error, code }) => {
		switch(code) {
			case StatusMap.Accepted:
				console.log(error)
		}
	})
	.get('/', () => {
		throw error("I'm a teapot")
	})
	.listen(3000)

@dtinth
Copy link
Author

dtinth commented Dec 27, 2024

@SaltyAom Thanks, I confirm that it works now, but now plain Error objects now return a 200 OK response instead of 500.

import { Elysia } from "elysia";
new Elysia()
  .get("/", async () => {
    throw new Error("nyan");
  })
  .listen(3000);
curl -D- localhost:3000
HTTP/1.1 200 OK
content-type: text/plain;charset=utf-8
Date: Fri, 27 Dec 2024 12:59:27 GMT
Content-Length: 4

nyan

@SaltyAom
Copy link
Member

SaltyAom commented Dec 27, 2024

@dtinth should have been fixed with 958fde0

Couldn't believe I missed that test case somehow

@just-dzhi
Copy link

thx for new Error('a')
https://youtube.com/shorts/PbIWVPKHfrQ

@dtinth
Copy link
Author

dtinth commented Dec 27, 2024

@SaltyAom Thanks, I confirm that the status is indeed fixed to be 500 now.

However, there are 2 behavior differences between these 2 versions:

Behavior 1.2.6 1.2.8
mapResponse called (twice!) not called at all now
Response body format JSON stringified
{"name":"Error","message":"crash"}
(but content-type is text/plain)
Just the message
crash

I’m okay either way, just want to make sure that you intend that not to call mapResponse

I think your project may benefit from having more snapshot testing that tests Elysia from a user’s (i.e. an outsider) perspective. I see that you use example/a.ts for testing various stuff, maybe you can snapshot the request and response, so that when behavior changed, you get an output diff in your commit which you can then review.

In my Elysia by Example project, I have a collection of example code and maintain a set of curl command and their expected output. That’s how I detected this issue right after you release a new version. Feel free to take the code if you’d like, although I’m still improving the examples and snapshot format.

Here’s the snapshot diff between v1.2.6 and v1.2.8:

image

@awilderink
Copy link

This behaviour is preventing me from using version 1.2 at this stage. I can't intercept the response in an .onError hook and remap it

@dtinth
Copy link
Author

dtinth commented Jan 21, 2025

@awilderink Can you share your example case? I'm curious to see how you use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants