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.

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

[SignalR] Remove obsolete APIs #28100

Merged
merged 1 commit into from
Jan 4, 2021
Merged

[SignalR] Remove obsolete APIs #28100

merged 1 commit into from
Jan 4, 2021

Conversation

BrennanConroy
Copy link
Member

Part of #27529

Both APIs already throw when used. The ParseResponse method was obsoleted in 3.1, HubInvocationContext.ctor was obsoleted in 5.0.

I have seen no evidence of anyone using HubInvocationContext.ctor and there wasn't a good reason to use it before 5.0 (Hub filters).

I have seen one place using ParseResponse, but the newest version of the package uses the non-obsolete method.

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Nov 23, 2020
@BrennanConroy BrennanConroy added this to the 6.0-preview1 milestone Nov 23, 2020
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Beautiful

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Normally, I would say why break it? But since this basically just changes a NotSupportedException to a MethodMissingException for libraries, and it makes what could be an annoying runtime error into a compile-time error for projects that are rebuilding anyway, this makes a lot of sense.

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

ghost commented Dec 15, 2020

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 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 Jan 4, 2021
@BrennanConroy BrennanConroy merged commit 5467bd5 into master Jan 4, 2021
@BrennanConroy BrennanConroy deleted the brecon/obs branch January 4, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants