-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Treat exceptions as 500 responses instead of a "Failed to fetch" error #516
Comments
Hi, @nzakas. Thanks for suggesting this.
The reason why throw a generic
Just like the native fetch, we preserve the actual error thrown in the
This changes the type of the error and deviates the fetch behavior compared to when you aren't using Interceptors. I don't believe this is the way forward. How we can improve thisI agree that's a suboptimal developer experience. On our end, I think to improve the developer experience, we can forward the original error to the console alongside the actual generic error: console.error(originalError)
throw new TypeError('Failed to fetch', { cause: originalError }) However, we cannot reliably reason about which errors you wish to see and which you don't. If you are mocking intentional server-side errors, you don't want to see those repeated in your console during the test run. This makes it hard for us to differentiate between these two errors, and this is the reason why I don't think we should do anything about them implicitly. How you can improve thisOn your end, you shouldn't put assertions in request handlers. Remember that you write request handlers from the server's perspective. This means that any error thrown in a handler is treated as an error thrown in the actual server processing your request. Naturally, it translates to the network. Instead, capture the resolver call and assert on the mock: const resolverListener = vi.fn()
server.use(http.get(url, resolverListener))
await waitFor(() => expect(resolverListener).toHaveBeenCalled())
expect(resolverListener).toHaveBeenCalledWith(
expect.objectContaining({
params: { folder_id: expectedFolderId },
// ...other assertions.
})
) To assert more complex structures, using asymmetric assertions can be tedious. Instead, you can unwrap the call to the mock function and assert on the data itself: const { params, request } = resolverListener.mock.calls[0][0]
// ...assertions. This is how I recommend solving this problem:
|
Ah, this makes sense, thanks for explaining.
I understand what you are saying in theory, but perhaps my view on what's happening is different. My understanding is that "failed to fetch" occurs in standard
With the first three, this occurs because of validation before the network request is made, while the fourth occurs basically says "you're not allowed to make this request", and the fifth means the server disappeared. None of these situations seems to be relevant to the case where an interceptor has an error during execution.
And this is where I'm confused. From the server's perspective, an error while processing a route would most likely not crash the server (no response sent, so "failed to fetch"), but rather result in a 500 error being sent back to the client. And if I was writing actual server logic and there was a crash, I would expect that error to be the one I received, not "failed to fetch", which is a client-side error. My expectation for MSW was that either of those two behaviors would occur when there's an error in an interceptor if the intent is to write interceptors as if they're the server. |
I think what you are saying makes sense. MSW by itself coerces these errors to 500 responses in the browser but I realized now that we don't do that for the Node.js counterpart. I think that's an oversight and we should fix it. I propose this:
This will actually create a more consistent behavior between environments. |
That sounds great. Thanks for being open to the feedback. 🙏 |
@nzakas, thanks for pointing out this inconsistency! It was always the intention to coerce unhandled exceptions to 500 server errors. Somehow, the Interceptors didn't implement that. I've opened a PR to fix this behavior. Unhandled exceptions will now result in 500 error responses, and to mock a request (network) error, one can use |
Since this is a breaking change, I will schedule it for the next minor version release. I may need to publish a few bugfixes around WebSockets until then. |
Released: v0.28.0 🎉This has been released in v0.28.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Right now, any error that occurs inside of an interceptor is wrapped in a network error. Depending on the system that is receiving the error, this can cause problems.
In my case, I'm running asserts inside of an interceptor, like this:
When any of the asserts fail, Mocha just displays "Failed to fetch", and that really destroys my productivity while debugging.
Not to mention, any custom properties on the thrown error aren't accessible on the wrapped error. That means the nice output that assertion errors typically create in the console are lost.
Suggestions
The text was updated successfully, but these errors were encountered: