Skip to content
This repository has been archived by the owner on Nov 9, 2023. It is now read-only.

WIP: Improve middleware errors #84

Draft
wants to merge 8 commits into
base: remove-next
Choose a base branch
from

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Mar 29, 2021

Pending: #83

This attempts to address some of the concerns raised in MetaMask/core#1993, regarding the throwing of non-Error values. It doesn't fully resolve the issue, because the async signatures of engine.handle will never reject, and will only pass through middleware errors as plain object clones, without a stack property. The callback signature of engine.handle receives the thrown error as the first argument, in the style of Node.js.

TODO

  • Figure out what to do with error stacks. See comments.

@rekmarks rekmarks requested a review from a team as a code owner March 29, 2021 00:39
@rekmarks rekmarks changed the base branch from main to remove-next March 29, 2021 01:57
@rekmarks
Copy link
Member Author

rekmarks commented Mar 29, 2021

I'm considering how we can return the full, native Error object to the caller via the Promise-based engine.handle signatures. We can't cause engine.handle to reject because we need to pass a JSON-RPC response to the caller. A few options come to mind:

  1. Preserve the stack property on the plain object clone. We could make this behavior configurable. We removed it in the first place to avoid sending useless production stacks to external domains.
  2. Assign the native Error object to response.error, and leave serialization to a different layer.
  3. Make the async signatures of engine.handle return something like Promise<[Error, response]>. I think I hate this, but will put it out there nonetheless.

@rekmarks rekmarks marked this pull request as draft March 29, 2021 04:44
@MajorLift
Copy link
Contributor

This library has now been migrated into the core monorepo. This PR has been locked and this repo will be archived shortly. Going forward, releases of this library will only include changes made in the core repo.

  • Please push this branch to core and open a new PR there.
  • Optionally, add a link pointing to the discussion in this PR to provide context.

@MetaMask MetaMask locked and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants