-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Not make the return type of a resolver void
#1671
Comments
FYI, the msw/src/handlers/RequestHandler.ts Lines 56 to 60 in e87e00b
|
Hi, @tnyo43. Thanks for proposing this. Historically, a resolver could not return anything if you wished to observe the request but don't affect it in any way. That changed since #1204, as now you have to explicitly tell MSW that you wish to perform the request as-is. However, to keep backward compatibility, the I thought we tackled it in #1404, but based on the source code, we didn't: msw/src/core/handlers/RequestHandler.ts Line 35 in 61f6f3b
Would you like to open a pull request that removes |
As I'm looking into the open pull request, I'm discovering that simply removing the In TypeScript, all of the following scenarios satisfy the
Those also have no distinction in JavaScript, since all three effectively represent |
Looks like my initial assumption about the return intentions from the resolver needs adjustments. I mention it here #1207 (comment) |
Sharing my thoughts on this. I do like the default fallthrough behavior. I like default behaviors in general, and this one in particular. If you define a list of handlers, a matching request will flow through them from top to bottom: rest.get('/resource', () => console.log('a')),
rest.get('/resource', () => console.log('b')),
rest.get('/resource', () => console.log('c')), I think this is a great feature of the library, and a part of it is that it's implicit. I'm usually not a fan of implicit (read "magic") but this may be a rare exception. Now, if we start demanding an explicit return all the time, this behavior loses part of its appeal, that being its succinctness. rest.get('/resource', () => {
console.log('a')
return fallthrough()
}),
rest.get('/resource', () => {
console.log('b')
return fallthrough()
}),
rest.get('/resource', () => {
console.log('c')
return fallthrough()
}), It becomes much more verbose to achieve the same thing. At this point, it's not an API question but a behavior question. Let's compare these two behaviors. Implicit fallthroughPros:
Cons:
Explicit return/fallthroughPros:
Cons:
With this comparison in mind, I'm slowly leaning towards changing nothing. There's a positive jing, a negative jing, and there's also a neutral jing. This may be the case when no change is the best change. But how do we guard against a forgotten return?Alas, we don't. I know it may bite sometimes but a missing
I know this can be improved but TypeScript doesn't give us any means to improve it without a significant sacrifice of the default behavior of the library. The gain is not worth the price here. I will close this issue but we can discuss it still, maybe someone will find good points that'd prove me wrong. I'd like that. |
Scope
Improves an existing behavior
Compatibility
Feature description
(I'm not sure if this can be a breaking change.)
With typescript, we currently can make a handler as like follows:
But also the following handler is valid in typescript even thought its return type is
void
.Is there any case that the return type of a handler should be
void
? Otherwise, I really appreciate if thatvoid
is omitted from the return type.I have made a handler as like follows and of course it didn't work.
This kind of problem is likely to happen but it actually took a lot of time to find the cause.
The text was updated successfully, but these errors were encountered: