Prevent fetch.pump recursively returning promises #184
Conversation
|
Thanks for this patch, please could you include a screenshot of the chrome memory profiler which demonstrates the new characteristics with this change. |
| @@ -22,14 +22,15 @@ class Fetch implements Transport { | |||
| this.options = transportOptions; | |||
| } | |||
|
|
|||
| pump(readerArg: ReadableStreamReader, res: Response): Promise<void | Response> { | |||
| pump(readerArg: ReadableStreamReader, res: Response) { | |||
| this.reader = readerArg; | |||
There was a problem hiding this comment.
Could you explicitaly type the return value, please?
There was a problem hiding this comment.
Pump no longer returns anything. The method now matches the send method within the class.
The previously returned promise was never actually used anywhere. The pump method was only called recursively, or from within the send method; and send has no return value.
All methods within this class that aren't returning anything are missing the void return type. I can update them all to void if that's helpful?
There was a problem hiding this comment.
All methods within this class that aren't returning anything are missing the void return type
🤔 Apologies I assumed we had explicit typing.
Given that we do not this boils down to personal preference so feel free to disregard my comment.
| @@ -40,7 +41,8 @@ class Fetch implements Transport { | |||
| detach(() => { | |||
| this.options.onChunk(result.value); | |||
| }); | |||
| return this.pump(this.reader, res); | |||
| this.pump(this.reader, res); | |||
| return; | |||
There was a problem hiding this comment.
I think this return statement is redundant and can be removed
There was a problem hiding this comment.
You're right, have removed.
There was a problem hiding this comment.
Removing this caused CI to fail - 'Not all code paths return a value'
There was a problem hiding this comment.
Oh dear that's 2/2 for bad suggestions from me 😂 Apologies for any time wasted and thanks again for the patch
|
@jonnyreeves added screenshots taken from issue 183. Thanks for your feedback 👍 |
nathanb21
force-pushed
the
bugfix/prevent-fetch-pump-promise-recursion
branch
from
ffd0827
to
5d56a6e
Compare
|
Good to merge. Will aim to action this and cut a release to npm either today or tomorrow, schedule permitting. Thanks again |
Fixes #183
Before:
After:
