-
Notifications
You must be signed in to change notification settings - Fork 52
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
Wrap error structure #1431
Wrap error structure #1431
Conversation
…cture # Conflicts: # packages/js/client/src/PolywrapClient.ts
…ts for wrap error structure
🔥 🔥 🔥 🔥 🔥 🔥 |
100-255 -> Unallocated | ||
*/ | ||
export enum WrapErrorCode { | ||
UNKNOWN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cases where we're reading from a buffer, and we get a zero there, I think the buffer should be treated as corrupted and not a valid WrapError. I do not think we should be allowing undefined states for an enum we control. The error codes, IMO, should start at 1.
Still, I am fine with keeping this for the time being and being more strict about the codes in the future.
Object.setPrototypeOf(this, WrapError.prototype); | ||
} | ||
|
||
static parse(error: string): WrapError | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's 2 things one can parse: serialized WrapErrors and errors from wasm modules (which may or may not be WrapErrors).
- The serialized WrapError parsing logic should be in the WrapError, or somewhere close. It is easy to parse since we know the structure and does not need regex.
- The wasm module error parsing logic should be in wasm-js. And since it can be anything regex is fine there.
The parsing logic here is both of those things.
Somehow I can't reply to some of your messages so I'm responding here.
I removed
All errors from Wasm modules are made to be of type It is possible that a message is not a stringified |
…ault code value; the `prev` property in WrapError was renamed to `innerError`; changed type of innerError to WrapError
We can address this in a followup PR when we use msgpack for error serialization |
Okay, sounds good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR adds a custom
WrapError
error structure that is capable of parsing errors from strings, allowing us to improve error formatting and provide additional information for debugging.Example error output for URI Resolution Exception:
Example error output for exception thrown during Wasm invocation:
Example error output for exception thrown during plugin wrapper invocation:
Closes #701