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

Change "Body is unusable" error to something more helpful #3097

Closed
nzakas opened this issue Apr 11, 2024 · 7 comments · Fixed by #3105
Closed

Change "Body is unusable" error to something more helpful #3097

nzakas opened this issue Apr 11, 2024 · 7 comments · Fixed by #3105
Labels
enhancement New feature or request

Comments

@nzakas
Copy link
Contributor

nzakas commented Apr 11, 2024

This would solve...

Unidici throws an error with the message "Body is unusable" when the body of a response is non-null and the stream is either locked or disturbed. In most cases, this situation occurs because someone has attempted to ready the body twice, such as:

const response = await fetch(url);
console.log(await response.text());
const body = await response.json();  // throws "Body is unusable"

The message "Body is unusable" is opaque and requires searching to understand what it means and what the problem is. A more descriptive message would save developers a lot of time.

The implementation should look like...

Changing the string on this line:

throw new TypeError('Body is unusable')

Suggestion: "Body is unusable: Body is either null or the stream has already been read."

The spec only specifies that a TypeError must be thrown, but it does not specify the text contained within that error, so it seems like changing the message would still be in-line with the spec.

I have also considered...

Not filing this issue. 😄

Additional context

@nzakas nzakas added the enhancement New feature or request label Apr 11, 2024
@mcollina
Copy link
Member

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@KhafraDev
Copy link
Member

I am not certain that the body can be null there

@nzakas
Copy link
Contributor Author

nzakas commented Apr 12, 2024

@mcollina happy to! 🎉

@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 12, 2024

If I had to implement it, I would do it like this:

// symbols.js
const kBodyOk = Symbol('body_ok')
const kBodyLocked = Symbol('body_locked')
const kBodyDisturbed = Symbol('body_disturbed')

// https://fetch.spec.whatwg.org/#body-unusable
function bodyUnusable (body) {
  // An object including the Body interface mixin is
  // said to be unusable if its body is non-null and
  // its body’s stream is disturbed or locked.
  if (body != null) {
    if (body.stream.locked) {
      return kBodyLocked
    } else if (util.isDisturbed(body.stream))
      return kBodyDisturbed
    }
  }
  return kBodyOk
}

then throw it properly in the linked code place. or directly throw in bodyUnusable. Maybe rename it to assertBodyUsable.
But probably @KhafraDev has a strong opinion regarding such a change.

@nzakas
Copy link
Contributor Author

nzakas commented Apr 12, 2024

@KhafraDev ah gotcha, I see that bodyUsable() returns false if body is null. 👍 So maybe just "Body is unusable: The stream has already been read."?

@KhafraDev
Copy link
Member

That message isn't much better, it assumes that you have some knowledge of fetch internals (that the body mixin methods are helper methods for consuming .body, which is a ReadableStream).

This example doesn't use any streams outwardly:

const response = await fetch('https://www.google.com')
res.text()
res.text() // TypeError: Body is unusable

I don't think "read" is the correct verbiage here either as it might imply that the entire body has been read, which also may not be true. Also, "read" is something you can do to a ReadableStream so it may confuse people who do not directly access .body.

const response = await fetch('https://www.goolge.com')
const reader = response.body.getReader()

await reader.read() // a single chunk

reader.releaseLock()

await response.text() // TypeError: Body is unusable

Instead, it should be something along the lines of "Body has already been consumed". Still, I don't necessarily think that the message is entirely clear and I suspect people will still have to look up what it actually means. Suggestions are welcome.

@nzakas
Copy link
Contributor Author

nzakas commented Apr 12, 2024

Honestly, anything in addition to "Body is unusable" would have saved me a ton of time. I've opened a PR. Happy to workshop the wording over there.

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

Successfully merging a pull request may close this issue.

4 participants