Skip to content

[SignalR TS] Pass timeout when invoking http requests #34207

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

Jump to bottom
Merged
BrennanConroy merged 4 commits into from
Aug 4, 2021
Merged

[SignalR TS] Pass timeout when invoking http requests #34207

BrennanConroy merged 4 commits into from
Aug 4, 2021

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Jul 8, 2021 •
edited
Loading

Fixes #27762

IHttpConnectionOptions
{
    skipNegotiation?: boolean;
    withCredentials?: boolean;
+   httpTimeout?: number;
}

@BrennanConroy BrennanConroy added area-signalr Includes: SignalR clients and servers feature-client-javascript Related to the SignalR JS client labels Jul 8, 2021
@BrennanConroy BrennanConroy requested a review from halter73 as a code owner July 8, 2021 17:50
davidfowl
davidfowl reviewed Jul 13, 2021
View reviewed changes
halter73
halter73 reviewed Jul 26, 2021
View reviewed changes
src/SignalR/clients/ts/signalr/src/IHttpConnectionOptions.ts Outdated
*
* This will not apply to Long Polling poll requests, EventSource, or WebSockets.
*/
defaultHttpTimeoutInMilliseconds?: number;
Copy link
Member

@halter73 halter73 Jul 26, 2021

Choose a reason for hiding this comment

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

I think we decided on httpTimeout for this, right?

Copy link
Member Author

@BrennanConroy BrennanConroy Jul 27, 2021

Choose a reason for hiding this comment

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

That or defaultTimeout?

Copy link
Member Author

@BrennanConroy BrennanConroy Jul 27, 2021

Choose a reason for hiding this comment

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

httpTimeout is better IMO, used that

Copy link
Member

@halter73 halter73 Jul 27, 2021

Choose a reason for hiding this comment

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

I also like requestTimeout since this is already on IHttpConnectionOptions, but I'm fine with it as is.

@halter73
Copy link
Member

halter73 commented Jul 27, 2021

Should we mark this api-ready-for-review?

@BrennanConroy BrennanConroy added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 27, 2021
@ghost
Copy link

ghost commented Jul 27, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@pranavkm
Copy link
Contributor

pranavkm commented Aug 4, 2021

API review:

-  httpTimeout?: number;
+ timeout?: number;

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 4, 2021
@BrennanConroy BrennanConroy merged commit 140a857 into main Aug 4, 2021
@BrennanConroy BrennanConroy deleted the brecon/timeoutts branch August 4, 2021 21:20
@ghost ghost added this to the 6.0-rc1 milestone Aug 4, 2021
@IlincaM
Copy link

IlincaM commented Oct 28, 2021

Can you please make this change on the @microsft/signalr libary too?

Sema Reaction: ❓ I have a question

@ghost
Copy link

ghost commented Oct 28, 2021

Hi @IlincaM. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@BrennanConroy
Copy link
Member Author

BrennanConroy commented Oct 28, 2021

Can you please make this change on the @microsft/signalr libary too?

That's what this change is for... you need to use a 6.0+ version to have the new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@davidfowl davidfowl davidfowl approved these changes

Assignees

No one assigned

Labels

api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers feature-client-javascript Related to the SignalR JS client

Projects

None yet

Milestone

6.0-rc1

Development

Successfully merging this pull request may close these issues.

SignalR client doesn't reconnect after server failure

6 participants

@BrennanConroy @halter73 @pranavkm @IlincaM @davidfowl