- 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