docs: expand mixins information, notate body reuse #1209
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1209 +/- ##
==========================================
+ Coverage 93.50% 94.27% +0.76%
==========================================
Files 43 45 +2
Lines 4035 4192 +157
==========================================
+ Hits 3773 3952 +179
+ Misses 262 240 -22
Continue to review full report at Codecov.
|
Co-authored-by: Robert Nagy <ronagy@icloud.com>
README.md
Outdated
|
|
||
| Should you need to access the `body` in plain-text after using a mixin, the best practice is to use the `.text()` mixin first, and manually parse the text to the desired format. | ||
|
|
||
| If the scenario arises where you absolutely must have a copy of the result stream, [`.clone`](https://fetch.spec.whatwg.org/#dom-request-clone) can be used _before_ using a `body` mixin, or [`body.pipeThrough`](https://streams.spec.whatwg.org/#readablestream-pipe-through) with a user-defined stream [§](https://web.dev/fetch-upload-streaming), such as [`TransformStream`]https://nodejs.org/api/stream.html#duplex-and-transform-streams. |
There was a problem hiding this comment.
cc @ronag how do you think this can be fixed?
There was a problem hiding this comment.
I mean that this refer to body.pipeThrough() about the http.request() while we do not have a WHATWG stream there.
There was a problem hiding this comment.
Like you suggested above. Mentions to web streams should be removed IMHO.
There was a problem hiding this comment.
That would really neuter the usefulness of the paragraph. I'd really like to retain something for users to point them in the right direction. Could you suggest an edit here?
There was a problem hiding this comment.
ReadableStream or anything web stream related is not relevant here. I think maybe you want to put another version of this under fetch?
There was a problem hiding this comment.
OK, I can remove it. I don't think it belong under fetch because users that need to copy the result stream while using undici (and that's the important part here) should have something to reference. I understand that from a maintainer's point of view it doesn't belong, but from a user's point of view, that takes away a resource of understanding.
If we can't canonize it in the docs, would it be acceptable to open a new issue as a "how to," paste the contents above in the issue, and then close it? So at the least there'd be something searchable in the repo.
There was a problem hiding this comment.
but from a user's point of view, that takes away a resource of understanding.
I'm sorry bit I'm not sure how it belongs here even from a user's point of view. I find it misleading since there is no web stream in this part of the API.
If we can't canonize it in the docs, would it be acceptable to open a new issue as a "how to," paste the contents above in the issue, and then close it? So at the least there'd be something searchable in the repo.
Sure.
|
Closing as stale. If you want to resolve the comments feel free to re-open. |
|
Fellas, this is a poor way to go about a PR. @ronag you've got an open conversation with @mcollina still pending. If it's stale, that's because you two fine gentleman haven't wrapped up that thread. Was happy to contribute, but please don't discount contributor efforts before maintainers have wrapped up discussion. |
Co-authored-by: Matteo Collina <matteo.collina@gmail.com>
* docs: expand mixins information, notate body reuse * chore: apply suggestions from code review Co-authored-by: Robert Nagy <ronagy@icloud.com> * docs: remove ref to web stream. Co-authored-by: Matteo Collina <matteo.collina@gmail.com> * docs: remove stream clone info * Update README.md Co-authored-by: Robert Nagy <ronagy@icloud.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com>
* docs: expand mixins information, notate body reuse * chore: apply suggestions from code review Co-authored-by: Robert Nagy <ronagy@icloud.com> * docs: remove ref to web stream. Co-authored-by: Matteo Collina <matteo.collina@gmail.com> * docs: remove stream clone info * Update README.md Co-authored-by: Robert Nagy <ronagy@icloud.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com>
* docs: expand mixins information, notate body reuse * chore: apply suggestions from code review Co-authored-by: Robert Nagy <ronagy@icloud.com> * docs: remove ref to web stream. Co-authored-by: Matteo Collina <matteo.collina@gmail.com> * docs: remove stream clone info * Update README.md Co-authored-by: Robert Nagy <ronagy@icloud.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com>
This PR proposes to resolve the documentation issue outlined in #1208.
I referenced the
fetchspec in some ways I felt read naturally, and attempted to structure the docs in a way that it would read/flow from most common use cases and wanted-info to most obscure or infrequently used. Added a little sugar to references that users might find useful or interesting. I'm not married to any part of it, but want to make sure that we're pointing users in the right direction for using mixins and potential pitfalls, especially those who have been using alternatives to fetch and aren't up to speed.