-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
fix(xhr): handle responses without body correctly #411
Conversation
* when constructing a Response instance. | ||
* @see https://github.com/mswjs/interceptors/issues/379 | ||
*/ | ||
const responseBodyOrNull = statusCodesWithoutBody.includes(request.status) |
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 moved this check to the createResponse()
function so it always produces the correct response no matter what XHR instance it's given. This logic belongs here.
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.
@avivasyuta, also note that representing the status codes that must not have a body in an array is more practical in this case than an object. At its core, we want a list of things, so an array is the right choice here!
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.
Yes, at first I thought that it would be more productive to use an object, but for three elements this can be neglected.
await httpServer.close() | ||
}) | ||
|
||
it('represents a 204 response without body using fetch api response', async () => { |
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.
Take a look, I've expanded the tests, creating three separate test cases instead of iterating over the list of status codes. It's crucial to keep the tests as flat and straightforward as possible. This means as little logic specific to tests as possible, like test.each()
. This results in tests that are easy to find, read, change, and remove.
|
||
const httpServer = new HttpServer((app) => { | ||
app.use(useCors) | ||
app.get('/:statusCode', (req, res) => |
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 made this server route return whichever response status code that was requested by the client.
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.
This is a great approach. I didn't think of it right away.
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 liked how you orchestrated the test! This is but a small adjustment.
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.
The changes look great! 👏 I've only moved the things around. Fantastic work done, @avivasyuta.
Yet another amazing contribution nailed. Let's ship this 🚀 |
Released: v0.24.1 🎉This has been released in v0.24.1! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
XMLHttpRequest
failing to constructResponse
when body status isnull
#379