- From: Adam Rice <notifications@github.com>
- Date: Tue, 08 May 2018 04:52:04 -0700
- To: whatwg/streams <streams@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <whatwg/streams/pull/924/review/118327884@github.com>
ricea commented on this pull request.
Thanks for spotting the problems and providing fixes.
> @@ -5692,14 +5692,27 @@ useful when there is instance data to track.
The class would be used in code like:
<pre><code class="lang-javascript">
+ function textDecoderStream() {
I would much rather use the TextDecoderStream class that is planned to be added to the encoding standard (https://github.com/whatwg/encoding/issues/72), even though it isn't available yet. It avoids spending a lot of space on this mostly-unrelated topic, and avoids data corruption when characters are split between chunk boundaries.
> const data = { userName, displayName, icon, date };
const ts = new TransformStream(new LipFuzzTransformer(data));
+ const { url } = fetchEvent.request;
Might be better not to have a `url` intermediate value, but just extract the value directly in the call to fetch().
>
fetchEvent.respondWith(
fetch(url).then(response => {
- const transformedBody = response.body.pipeThrough(ts);
+ // Decode the binary-encoded response to string
+ const decodedBody = response.body.pipeThrough(textDecoderStream());
It's better to not create a `decodedBody` variable, but just chain the pipes like this
```javascript
const transformedBody = response.body
.pipeThrough(new TextDecoderStream())
.pipeThrough(ts);
```
I don't think it's necessary to comment each operation separately, but you can if you think it is clearer.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/924#pullrequestreview-118327884
Received on Tuesday, 8 May 2018 11:52:26 UTC