test: remove unnecessary .toString() calls in HTTP tests - #43731
Conversation
Sorry, something went wrong.
Let’s not have bad examples in our test suite and instead use the
proper way of converting stream data to UTF-8
(i.e. `stream.setEncoding('utf8')`) in all places.
Sorry, something went wrong.
| response += chunk; | ||
| })); | ||
|
|
||
| client.setEncoding('utf8'); |
There was a problem hiding this comment.
Nit(feel free to ignore): would be better to move client.setEncoding('utf8'); before client.on('data'?
Reason:
- keep consistent with other use cases
- don't make us guess if client.setEncoding('utf8') after attach data listener would cause problem.
Sorry, something went wrong.
There was a problem hiding this comment.
Fwiw, my preferred way is client.setEncoding('utf8').on('data', ...') 😄
Ultimately, it doesn't matter. If we do clean this up, it might also be a good idea to do so across all tests for consistency.
Sorry, something went wrong.
|
Maybe it is time to doc-deprecate buffer.toString() params at all? |
Sorry, something went wrong.
I don't think that that's practical or meaningful. We should just push really hard to avoid examples like these so that people know not to do it this way 🙂 Also, this is something that TypeScript would have caught at least in the cases where .setEncoding() is already being used. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Let’s not have bad examples in our test suite and instead use the
proper way of converting stream data to UTF-8
(i.e. `stream.setEncoding('utf8')`) in all places.
PR-URL: #43731
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Let’s not have bad examples in our test suite and instead use the
proper way of converting stream data to UTF-8
(i.e. stream.setEncoding('utf8')) in all places.