Skip to content
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.

Sign up for GitHub

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

Prevent fetch.pump recursively returning promises #184

Merged
merged 1 commit into from
Apr 30, 2018
Merged

Prevent fetch.pump recursively returning promises #184

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions ts/src/transports/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explicitaly type the return value, please?

Copy link
Contributor Author

@nathanb21 nathanb21 Apr 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (this.cancelled) {
// If the request was cancelled before the first pump then cancel it here
this.options.debug && debug("Fetch.pump.cancel at first pump");
return this.reader.cancel();
this.reader.cancel();
return;
}
return this.reader.read()
this.reader.read()
.then((result: { done: boolean, value: Uint8Array }) => {
if (result.done) {
detach(() => {
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this return statement is redundant and can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, have removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this caused CI to fail - 'Not all code paths return a value'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dear that's 2/2 for bad suggestions from me 😂 Apologies for any time wasted and thanks again for the patch

});
}

Expand All @@ -56,7 +58,8 @@ class Fetch implements Transport {
this.options.onHeaders(new Metadata(res.headers as any), res.status);
});
if (res.body) {
return this.pump(res.body.getReader(), res)
this.pump(res.body.getReader(), res)
return;
}
return res;
}).catch(err => {
Expand Down