fix: error on uncompressed datastreams when chunks are missing - #2073
fix: error on uncompressed datastreams when chunks are missing#2073lukasIO wants to merge 12 commits into
Conversation
🦋 Changeset detectedLatest commit: 57aba77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
lukasIO
changed the title
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…nt-sdk-js into lukas/fix-data-stream-order
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Generally looks good, I think before merging it would be good to get this also fixed for text streams and add test(s) to make sure that this doesn't regress.
@lukasIO If you'd like me to take this over let me know. I will also see if this is an issue in the rust implementation, and if so, open a PR fixing it over there.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ); | ||
| manager.handleDataStreamPacket(chunkPacket(streamId, 0, textBytes, 0), Encryption_Type.NONE); | ||
| // Chunk-level `version` retcon is not supported: a reader that has already yielded chunk 0 to | ||
| // its consumer cannot retract it, so a resend at the same index is dropped like any other | ||
| // duplicate rather than superseding the original. See the note on | ||
| // `TextStreamReader.handleChunkReceived`. | ||
| manager.handleDataStreamPacket( | ||
| chunkPacket(streamId, 0, new TextEncoder().encode('goodbye world'), 1), | ||
| Encryption_Type.NONE, | ||
| ); | ||
| manager.handleDataStreamPacket(trailerPacket(streamId), Encryption_Type.NONE); |
There was a problem hiding this comment.
nitpick: It also might be good to add a test here which sends chunk version 1 and then 0. From our conversation on slack, this is now "first one wins", so assuming I have that right I think it would be good to test both permutations.
| lastChunkIndex = index; | ||
| if (value.content.length === 0) { | ||
| return; | ||
| } | ||
| controller.enqueue(value); |
There was a problem hiding this comment.
(Just wanted to say explicitly that I think moving this to the send path instead of the receive path is probably the better place for this to live!)
2 participants
closes #2072