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.

Sign up for GitHub

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

Jump to bottom

Improve error handling #157

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improve error handling #157

wants to merge 2 commits into from

Conversation

njaremko
Copy link

@njaremko njaremko commented Mar 22, 2022 •
edited

Improves error handling by:

  • Adding support for anyhow
  • Implementing From<Error> for Response

Note: After making these changes, I get the following error when I run wrangler dev, which seems to suggest an issue with the bundler? Because I don't think I touched anything relevant to that error?

Error: Something went wrong with the request to Cloudflare...
Uncaught Error: No such module "snippets/wasm-streams-8c20110b5d812e48/inline0.js".
  at line 0
 [API code: 10021]

@njaremko njaremko changed the title Initial Improve error handling Mar 22, 2022
@njaremko njaremko marked this pull request as ready for review March 22, 2022 18:23
zebp
zebp requested changes Mar 28, 2022
View reviewed changes
Copy link
Collaborator

@zebp zebp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an improvement, but unsure on if all of these changes are necessary. It also might be good if we refactor StringError and make concrete error types for it's uses, but that's something I need to think over.

worker-macros/src/event.rs Outdated
quote! {
::worker::Response::error(e.to_string(), 500).unwrap().into()
quote! {
::worker::console_log!("{}", e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably keep the 500 response.

worker/src/error.rs
ParseError(#[from] url::ParseError),
StringError(String),
SerdeJsonError(#[from] serde_json::Error),
RustError(#[from] anyhow::Error),
Copy link
Collaborator

@zebp zebp Mar 28, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RustError is never used, it should probably be removed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in this project, but it lets projects building on this use anyhow

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, fair. But I think we should add anyhow as a dependency when we could just use Box<dyn std::error::Error>.

worker/src/error.rs Outdated
}

#[inline]
pub fn unwrap_abort<T, E>(o: Result<T, E>) -> T {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this needs to exist, I think just regular expect calls would be better. I might not be understanding correctly though, why this instead of an expect?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was pulled from https://rustwasm.github.io/book/reference/code-size.html#optimizing-builds-for-code-size . But yes, I agree that expect makes more sense.

worker/src/error.rs Outdated

impl From<Error> for worker_sys::Response {
fn from(e: Error) -> Self {
match e {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should just convert the error to a Response then .into() it into a sys response.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, fixed

njaremko
njaremko commented Apr 15, 2022
View reviewed changes
Copy link
Author

@njaremko njaremko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I took so long to address these, notification got buried with work stuff, and I forgot to check back 😅

worker/src/error.rs
ParseError(#[from] url::ParseError),
StringError(String),
SerdeJsonError(#[from] serde_json::Error),
RustError(#[from] anyhow::Error),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in this project, but it lets projects building on this use anyhow

worker/src/error.rs Outdated
}

#[inline]
pub fn unwrap_abort<T, E>(o: Result<T, E>) -> T {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was pulled from https://rustwasm.github.io/book/reference/code-size.html#optimizing-builds-for-code-size . But yes, I agree that expect makes more sense.

worker/src/error.rs Outdated

impl From<Error> for worker_sys::Response {
fn from(e: Error) -> Self {
match e {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, fixed

@njaremko njaremko requested a review from zebp April 15, 2022 05:03
zebp
zebp requested changes May 14, 2022
View reviewed changes
Copy link
Collaborator

@zebp zebp left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey sorry it took so long to review this, it also got lost in a sea of GitHub notifications. A few small changes and then this should be good to go! There's also a merge conflict and with a PR this small a rebase should be pretty easy.

worker/src/error.rs
ParseError(#[from] url::ParseError),
StringError(String),
SerdeJsonError(#[from] serde_json::Error),
RustError(#[from] anyhow::Error),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, fair. But I think we should add anyhow as a dependency when we could just use Box<dyn std::error::Error>.

worker/src/error.rs
RustError(#[from] anyhow::Error),
}

impl From<Error> for Response {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be opinionated on what an error response looks like, I think we should remove these impls. For stuff like this I think it's better for the user to have some way to turn errors into response with some structure they would like.

worker-macros/src/event.rs
::worker::Response::error(e.to_string(), 500).unwrap().into()
}
}
false => {
quote! { panic!("{}", e) }
quote! { e.into() }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we should remove the From<Error> for Response, we should go back to panicking.

@arbfay arbfay mentioned this pull request May 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zebp zebp zebp requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@njaremko @zebp