fix: properly handle child process stdio chunking

[addaleax]

Converting individual chunks from UTF-8 to JS strings
is problematic because it does not handle UTF-8 characters
that are split across chunks properly.

Use the proper way of reading string data from streams instead.

[addaleax]

@dsanders11 Just for context, I don’t have merge access here :)

[dsanders11]

@addaleax, noted. 🙂 I'm not familiar with this codebase so I don't want to merge this without getting another approval, although it looks fine on face value. I'll ping @malept.

[malept]

Is there a testcase for this? I need to port this to @malept/cross-spawn-promise I think (which will eventually replace this file in #373).

[addaleax]

@malept This is a standard bugfix that has happened in many places before. It is even explicitly mentioned in the Node.js streams docs for setEncoding():

The Readable stream will properly handle multi-byte characters delivered through the stream that would otherwise become improperly decoded if simply pulled from the stream as Buffer objects.

I can add a test case but it doesn’t seem that there are tests specifically for this file. This would be an example test case that could be added to your library, though:

import spawnPromise from './spawn-promise';
const result = await spawnPromise('node', ['-e', 'process.stdout.write(Buffer.from([0xe5, 0xa5])); setTimeout(() => process.stdout.write(Buffer.from([0xbd])), 20)']);
expect(result).to.equal('好'); // adjust to your favorite assertion library

(It may look contrived to write the bytes of a multibyte character individually. The more common form of this bug is that a character just happens to end up on a chunk boundary when a medium-to-large-size chunk of data is being written/read, but that’s less reliable to test because the chunk boundaries are not necessarily consistent across OSes, Node.js versions, etc., so a timer will generally do a better job for testing.)

[VerteDinde]

Approved pending merge conflict resolving - just resolved it, this looks good to me

[continuous-auth]

🎉 This PR is included in version 5.2.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀