fix: clone methods correctly handle teeing body stream #17
Conversation
|
Was hoping the tests would work in CI since I can't get them to run locally but looks like they're running into similar environment problems |
slightlytyler
force-pushed
the
clone-stream-bug
branch
from
ad40a47
to
436406d
Compare
| @@ -5,7 +5,8 @@ | |||
| "requires": true, | |||
| "packages": { | |||
| "": { | |||
| "version": "1.0.2", | |||
| "name": "react-native-fetch-api", | |||
There was a problem hiding this comment.
idk why this is changing when i npm install
| @@ -228,6 +233,16 @@ class Body { | |||
| }, | |||
| }); | |||
| } | |||
|
|
|||
| clone() { | |||
There was a problem hiding this comment.
| @@ -85,7 +85,14 @@ class Request { | |||
| } | |||
|
|
|||
| clone() { | |||
There was a problem hiding this comment.
| @@ -21,12 +21,19 @@ class Response { | |||
| } | |||
|
|
|||
| clone() { | |||
There was a problem hiding this comment.
| @@ -85,7 +85,14 @@ class Request { | |||
| } | |||
|
|
|||
| clone() { | |||
| return new Request(this, { body: this._body._bodyInit }); | |||
| if (this.bodyUsed) { | |||
| throw new TypeError("Already read"); | |||
There was a problem hiding this comment.
| @@ -21,12 +21,19 @@ class Response { | |||
| } | |||
|
|
|||
| clone() { | |||
| return new Response(this._body._bodyInit, { | |||
| if (this.bodyUsed) { | |||
| throw new TypeError("Already read"); | |||
There was a problem hiding this comment.
| @@ -46,7 +46,7 @@ class Request { | |||
| this.signal = request.signal; | |||
| this.headers = new Headers(options.headers ?? request.headers); | |||
|
|
|||
| if (!options.body && request._body._bodyInit) { | |||
| if (options.body === undefined && request._body._bodyInit) { | |||
There was a problem hiding this comment.
need to avoid explicit null body triggering this case I think
slightlytyler
changed the title
slightlytyler
force-pushed
the
clone-stream-bug
branch
from
8a06df0
to
0d9cad2
Compare
|
@acostalima are you still maintaining? Code here is good to go but the test infra is broken locally and in CI. I got it working locally by editing the generated rn test app but unsure of what the real fix is. seems like a problem with react-native-test-runner rather than config in this project though? happy to do whatever needed to get this merged in 👍 |
Encountered error
This stream has already been locked for exclusive reading by another readerwhen cloning a response that gets read as a stream. Problem was we did not handle teeing the body stream when cloning happens. The fix is:Also added a few updates to correct the behavior of this library wrt the spec. Namely body class handles explicitly null bodyInit and request/response clone methods check if the body has been used before executing.
Test cases added / modified to verify the new cloning behavior