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

Update fetch middleware to preserve original error data #349

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sterlu
Copy link

@sterlu sterlu commented Dec 10, 2024

Recently the rpc-errors package had been updated to preserve the original RPC error message (MetaMask/rpc-errors#160). However, the fetch middleware in this package has been preventing this as it has not been forwarding the body.error.message string from the response, instead rewriting all errors with the default 'Internal JSON-RPC error.'. This PR addresses that.

The data could also be passed explicitly as rpcErrors.internal({ message: body.error.message, data: body.error.data }) but the approach of passing the whole body.error has the advantage of parseOpts in getJsonRpcError handling the case in which if body.error is not an object but a string.

I wanted to add tests to cover this, but the only way I see this being done is by exporting the parseResponse method. Let me know if this is something you'd like covered.

@sterlu sterlu requested a review from a team as a code owner December 10, 2024 11:15
@mcmire
Copy link
Contributor

mcmire commented Dec 11, 2024

Hi @sterlu, thanks for this PR. We will take a look at it soon!

@legobeat
Copy link
Contributor

legobeat commented Dec 14, 2024

I wanted to add tests to cover this, but the only way I see this being done is by exporting the parseResponse method. Let me know if this is something you'd like covered.

Rather than unit-testing the internal behavior of this this otherwise unexported function, seems more appropriate to extend fetch.test.ts with tests that instantiate a fetch-middleware using createFetchMiddleware, then testing for that errors result in the expected (serializable?) result? Some similar tests already exist for retryOnEmptyMiddleware (e.g. #154, #254).

but the approach of passing the whole body.error has the advantage of parseOpts in getJsonRpcError handling the case in which if body.error is not an object but a string.

In particular I guess it would benefit from a regression test covering the eventuality of input error objects being cyclical, in order to cover for situations like:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants