-
-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade get-stream
#571
Upgrade get-stream
#571
Conversation
9640249
to
ab2f8f3
Compare
test/stream.js
Outdated
@@ -17,6 +17,25 @@ test('buffer', async t => { | |||
t.is(stdout.toString(), 'foo'); | |||
}); | |||
|
|||
const checkEncoding = async (t, encoding) => { | |||
const encodedString = '\u1000.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string was chosen to give different outputs with each encoding type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write that as a short code comment as it's definitely not clear when just reading the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in cff527d 👍
if (encoding) { | ||
return getStream(stream, {encoding, maxBuffer}); | ||
// eslint-disable-next-line unicorn/text-encoding-identifier-case | ||
if (encoding === 'utf8' || encoding === 'utf-8') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allows both utf8
and utf-8
to mimic Node.js behavior.
Note: this is already Execa's current behavior before this PR.
|
||
const applyEncoding = async (stream, maxBuffer, encoding) => { | ||
const buffer = await getStreamAsBuffer(stream, {maxBuffer}); | ||
return buffer.toString(encoding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one subtle change of behavior here (that is quite of an edge case): when using both maxBuffer
and an unusual encoding
(that is neither utf8
nor null
, such as hex
or base64
), then the number of bytes being limited by maxBuffer
is using stdout/stderr as a Buffer
, not as the final value of childProcess.stdout/stderr
, which is an hex/base64/etc.-encoded string.
This is due to get-stream
dropping support for the encoding
option, i.e. it can only apply maxBuffer
on the stream as Buffer
.
test('can pass encoding "ascii"', checkEncoding, 'ascii'); | ||
test('can pass encoding "hex"', checkEncoding, 'hex'); | ||
test('can pass encoding "base64"', checkEncoding, 'base64'); | ||
test('can pass encoding "base64url"', checkEncoding, 'base64url'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure we support all the encoding
option's value that childProcess.exec()
supports.
@sindresorhus Would you like me to make a major release? |
Yeah, go ahead 👍 |
Done in |
This upgrades
get-stream
to its latest version.