-
Notifications
You must be signed in to change notification settings - Fork 6
fix(errors): Properly assign properties on hardened ocap errors #173
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
Conversation
| @@ -0,0 +1,16 @@ | |||
| import '@ocap/shims/endoify'; | |||
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.
I know we’re already importing it via the setupFiles option in the Vitest config, but I’m explicitly importing it here for clarity. It doesn’t hurt, right? :)
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.
If clarity is good here, would it not also be good in the other tests, too? :)
For my part I think this is a fine canary to sing about changes to vitest.config.ts and nothing need be changed. I just don't think it's a clarity issue, and we can reasonably expect reviewers and developers to be familiar with the contents of setupFiles in the package's vitest.config.ts.
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.
You are not wrong :) I guess it's not needed.
grypez
left a comment
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.
Small invitations for discussion, but LGTM.
| export function unmarshalErrorOptions( | ||
| marshaledError: MarshaledError, | ||
| ): ErrorOptionsWithStack { | ||
| const output: ErrorOptionsWithStack = { stack: marshaledError.stack ?? '' }; |
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.
| const output: ErrorOptionsWithStack = { stack: marshaledError.stack ?? '' }; | |
| const output: ErrorOptionsWithStack = { stack: marshaledError.stack }; |
The type you define allows stack to be undefined. Why do you prefer to default to the empty string?
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.
Right, I wrote it better.
| @@ -0,0 +1,16 @@ | |||
| import '@ocap/shims/endoify'; | |||
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.
If clarity is good here, would it not also be good in the other tests, too? :)
For my part I think this is a fine canary to sing about changes to vitest.config.ts and nothing need be changed. I just don't think it's a clarity issue, and we can reasonably expect reviewers and developers to be familiar with the contents of setupFiles in the package's vitest.config.ts.
d367577 to
f5deb5a
Compare
closes #169
This PR addresses the issue where Ocap errors had their properties
stackandcauseassigned incorrectly after hardening, which went unnoticed in tests due to mocking of the harden function.The properties
stackandcauseare now assigned before hardening the error.The
unmarshalErrorOptionsfunction is introduced to properly unmarshal and assign the error options, ensuring that the properties are correctly handled during the unmarshaling process.Refactored
unmarshalErrorto ensure the correct unmarshaling of properties before hardening.Custom error classes adjusted to utilize
unmarshalErrorOptionswhen unmarshaling error instances.The tests have been updated to remove the mocked harden function, ensuring that the properties are correctly assigned and the hardened objects behave as expected.