Uses explicit null as 204 response body when handling the "response" worker event

[kettanaito]

Motivation

When handling a 204 status (actual) response, it's expected for it to have no body. However, there is no way to check if a response has no body given an arbitrary potentially 204 response:

await potentially204Response.text() // "" (same as a response with the explicit "" response text)
await potentially204Response.body // ReadableStream, locked and cannot be read

Looks like we need a safe-guard on our side to prevent the construction of a Response instance with an empty string body in case of a 204 status response:

new Response('', { status: 204 }) // Error
new Response(null, { status: 204 }) // OK

Changes

• Uses null in case the response status is 204, regardless of what its actual body is (in that case it's always null).
• Adds an integration test that asserts 204 status responses to be handled without page errors through the entire request/response life-cycle.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4d596ee:

Sandbox	Source
MSW React	Configuration

[Aprillion]

I found https://fetch.spec.whatwg.org/#statuses => IMHO let's check all null body statuses here:

Suggested change

	const responseBody = responseJson.status === 204 ? null : responseJson.body
	const responseBody = [101, 204, 205, 304].includes(responseJson.status) ? null : responseJson.body

if this is hot-path for perf optimization, maybe a new Set(...) in some util file can be used 😂

[kettanaito]

That's a great suggestion, but also a great example of why reflecting a spec this way is a huge redundancy. I wish to find a different way to retrieve a 204 response body as null.

[Aprillion]

yeah, no ideas about the clean way yet :/

[Aprillion]

well.. I do have an idea - responseJson.body === '' ? null : responseJson.body - but totally not sure how that would break all edge cases with empty body in 200 and other responses 😬

[kettanaito]

What about the responses that have an empty string body? There must be a differentiation between them as those responses that have no body (null). With the logic above, both cases will result in res.body being null.

[Aprillion]

yeah, this distinction between null and empty string is really strange, no idea what was the reason to make it different in the spec :(

[comatory]

Can I use this suggested change as a stop-gap for now? I'm using only msw for local development and it'd be nice to not have that warning in console (I'm using patch-package for these purposes).

[kettanaito]

@comatory, I'd rather recommend you edit the mockServiceWorker.js with a similar change instead. Changing what lies in node_modules isn't a reliable way to fix anything, as the modules will be pruned once you re-install the dependencies.

If you're using some tool to ensure the persistent change in a node module across installs, then you can add this suggestion into msw directly.

[comatory]

@kettanaito I see. However with patch-package the changes are re-applied after installation so it's safe (and actually recommended way) to do it. I'm fine with this solution, once this problem is fixed, I'll just remove the patch from my codebase.

[kettanaito]

After looking into the ways to differentiate between an empty string response and a null response, I've tried to read the res.body stream using a custom reader:

function readStream(stream) {
  return new Promise((resolve, reject) => {
    let result = null

    if (stream.locked) {
      return resolve(result)
    }

    const reader = stream.getReader()
    reader
      .read()
      .then(function processText({ value, done }) {
        if (done) {
          reader.releaseLock()
          return resolve(result)
        }

        const resolvedValue =
          typeof value === 'string'
            ? value
            : new TextDecoder('utf-8').decode(value)

        result = (result || '') + resolvedValue
        reader.read().then(processText)
      })
      .catch(reject)
  })
}

And then reading the stream:

const responseBody = await readStream(clonedResponse.body)

While working with ReadableStream, I've observed that for a readable stream there is no difference whether you stream an empty string or null—stream would be marked as "done" immediately (the result variable won't be ""). This got me thinking that it's possible to assume that an empty string response is a null body response, in terms of the body it gets.

We can fallback to null in case the response body is an empty string:

new Response(responseJson.body || null, responseJson)

By doing so:

• res.text() will still produce "" for both empty string response and a null body response.
• res.body will be null as opposed to ReadableStream if it's an empty string response. That's the only deviation there is.

[kettanaito]

I've settled on the body || null fallback when constructing a Response instance to pass to the response life-cycle events. That produces a response instance compatible with an actual instance. The only difference is that instead of a ReadableStream that streams nothing it sets res.body to null. Imho, that's a more reproducible behavior:

const res = new Response(null, { status: 204 })
res.body // null, as expected

[kettanaito]

@Aprillion, @marcosvega91, may I please ask for your review on this? GitHub's "re-request review" is broken for me. Thanks.

[marcosvega91]

Your observation is right, I think is great 💪 Great!!

[Aprillion]

I've settled on the body || null fallback when constructing a Response instance

I just checked await new Response(null, {status: 200}).text() is an empty string and not an exception, so it should be fine 👍

[kettanaito]

This is rather brave, but when handling a Response instance there is no difference between working with a new Response(null) and new Response('') in terms of the operable response body (i.e. .json/.text/etc.).