Skip to content

Conversation

analogrelay
Copy link
Contributor

@analogrelay analogrelay commented Mar 19, 2020

Fixes #10756

In 3.0 we switched SignalR to use endpoint routing and obsoleted the old extension methods. This PR removes them.

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.

+22 -403 👍

@@ -29,7 +29,6 @@ public async Task DetectFeaturesAsync_FindsNoFeatures()
}

[Theory]
[InlineData(nameof(StartupWithUseSignalR))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a provision for building the analyzer tests against older runtimes, though it might be feasible with TFM pivots. Doesn't feel super necessary though.

@analogrelay analogrelay added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-signalr Includes: SignalR clients and servers labels Mar 19, 2020
@ghost
Copy link

ghost commented Mar 19, 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.

@analogrelay analogrelay self-assigned this Mar 19, 2020
@mkArtakMSFT mkArtakMSFT added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Mar 23, 2020
@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 Mar 23, 2020
@pranavkm
Copy link
Contributor

Assert.Equal() Failure

↓ (pos 109)

Expected: ···erviceCollection.AddSignalR' inside the call to 'ConfigureSer···

Actual: ···erviceCollection.AddRouting' inside the call to 'ConfigureSer···

↑ (pos 109)


Stack trace
   at Microsoft.AspNetCore.SignalR.Tests.MapSignalRTests.<>c__DisplayClass1_0.<NotAddingSignalRServiceThrows>b__0(IApplicationBuilder app) in /_/src/SignalR/server/SignalR/test/MapSignalRTests.cs:line 49

@analogrelay
Copy link
Contributor Author

@pranavkm dorp. I will do the needful ;).

@analogrelay
Copy link
Contributor Author

I think this was because I incorrectly ported the NotAddingSignalRServiceThrows test to endpoint routing. Instead, I deleted it and renamed the old NotAddingSignalRServiceThrowsWhenUsingEndpointRouting test, since we don't need both anyway ;).

@ghost
Copy link

ghost commented Mar 23, 2020

Hello @anurse!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 0d80ffa into master Mar 23, 2020
@ghost ghost deleted the anurse/10756-remove-obsolete-signalr branch March 23, 2020 20:26
This pull request was closed.
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 breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove obsolete methods/classes in SignalR
5 participants