Fix maxBuffer bug with TextDecoder()
#105
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ehmicky
commented
Aug 16, 2023
| } | ||
|
|
||
| return finalize(contents, length, textDecoder); | ||
| appendFinalChunk({state, convertChunk, getSize, addChunk, getFinalChunk, maxBuffer}); |
There was a problem hiding this comment.
The final chunk (�) must use the same logic as the previous chunks, including the maxBuffer logic.
To do this required two changes:
- Adding a
getFinalChunk()method that returns eitherundefined(no final chunk) or the final chunk, with the right type - Extracting the
appendChunk()logic so that it can be used aftergetFinalChunk(). To do this, the inside of the mainforloop had to be extracted to its own functionappendChunk(). To do this, stateful arguments had to be put into astateobject. Changing this required refactoring some additional functions.
ehmicky
commented
Aug 16, 2023
| @@ -112,6 +112,11 @@ test('get stream with truncated UTF-8 sequences', async t => { | |||
| t.is(result, `${multiByteString.slice(0, -1)}${INVALID_UTF8_MARKER}`); | |||
| }); | |||
|
|
|||
| test('handles truncated UTF-8 sequences over maxBuffer', async t => { | |||
There was a problem hiding this comment.
This test succeeds with the current PR, and fails without it.
ehmicky
commented
Aug 16, 2023
source/array-buffer.js
Outdated
|
|
||
| export async function getStreamAsArrayBuffer(stream, options) { | ||
| return getStreamContents(stream, arrayBufferMethods, options); | ||
| } | ||
|
|
||
| const initArrayBuffer = () => new Uint8Array(0); | ||
| const initArrayBuffer = () => ({contents: new Uint8Array(0)}); |
There was a problem hiding this comment.
The changes in this file and the following ones are related to refactoring stateful variables into a state object. See comment below.
ehmicky
commented
Aug 16, 2023
| @@ -77,5 +77,6 @@ const arrayBufferMethods = { | |||
| }, | |||
| getSize: getLengthProp, | |||
| addChunk: addArrayBufferChunk, | |||
| getFinalChunk: noop, | |||
There was a problem hiding this comment.
getFinalChunk() is only used by getString(). It is a noop otherwise.
|
Feel free to do another release when you are ready. You have publish access on npm. |
|
Sounds good! Will do after #106. |
|
Done in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the stream contains a UTF-8 string with a partial multibyte sequence at the end, a final character
\ufffd(�) is appended. This is the standard behavior byTextDecoderand other tools.At the moment, this final byte is not checked by the
maxBufferlogic. I.e. the returned string could be one byte overmaxBufferwhen appending�.This PR fixes this bug. This required some refactoring.