Skip to content

SignalR: Reference shared framework #28398

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
merged 16 commits into from
Feb 24, 2023
Merged

SignalR: Reference shared framework #28398

merged 16 commits into from
Feb 24, 2023

Conversation

wadepickett
Copy link
Contributor

@wadepickett wadepickett commented Feb 15, 2023 •
edited
Loading

@wadepickett wadepickett self-assigned this Feb 15, 2023
@Rick-Anderson
Copy link
Contributor

This should go in the migration doc. This doc note can be far shorter with a link to the migration doc.

Rick-Anderson
Rick-Anderson reviewed Feb 15, 2023
View reviewed changes
@Rick-Anderson
Copy link
Contributor

@wadepickett review URL?

Rick-Anderson
Rick-Anderson reviewed Feb 15, 2023
View reviewed changes
@wadepickett
Copy link
Contributor Author

wadepickett commented Feb 16, 2023 •
edited
Loading

@Rick-Anderson, this is a draft. I promise I will ask for a review when it is ready for a review. Thanks for any suggestions, but please wait until I ask for a review, or at least until I take it out draft.
image

@Rick-Anderson
Copy link
Contributor

Rick-Anderson commented Feb 16, 2023 •
edited
Loading

Right, but it's not like you would have made those changes when you were ready for a review. You were obviously going down the wrong path so I wanted to save you some time and make us more efficient. We don't have much time to trim down our technical debt, we'll soon be busy with .NET 8 pre-releases. We need to work more efficiently.

image

@wadepickett
Copy link
Contributor Author

OK, I understand. You don't think I would have made those changes before a review, and you were trying to save me some time.

@wadepickett
Copy link
Contributor Author

wadepickett commented Feb 17, 2023 •
edited
Loading

@wadepickett review URL?

It is a draft. It was not made available for review. I drop that in as soon as I check the build and before putting up for review.

@Rick-Anderson
Copy link
Contributor

@wadepickett review URL?

It is a draft. It was not made available for review. I drop that in as soon as I check the build and before putting up for review.

I prefer that you put it in after you get a build along with Fixes #xyz.

@wadepickett
Copy link
Contributor Author

@wadepickett review URL?

It is a draft. It was not made available for review. I drop that in as soon as I check the build and before putting up for review.

I prefer that you put it in after you get a build along with Fixes #xyz.
I understand your preference.
#28339

wadepickett and others added 2 commits February 17, 2023 18:52
@wadepickett @Rick-Anderson
Update aspnetcore/signalr/hubs.md
b4d5dc0 
Committing the inline suggestion, and then will update the missing link afterwards.

Co-authored-by: Rick Anderson <[email protected]>
@wadepickett
Moved 7.0 version up, moved new note to bottom of topics
d7739d2
@wadepickett
Copy link
Contributor Author

Something up with the build. Closing and reopening to re-queue for build.

@wadepickett wadepickett reopened this Feb 18, 2023
@wadepickett
Copy link
Contributor Author

re build

@wadepickett wadepickett reopened this Feb 18, 2023
Rick-Anderson
Rick-Anderson reviewed Feb 19, 2023
View reviewed changes
@wadepickett wadepickett mentioned this pull request Feb 20, 2023
Open
@wadepickett wadepickett marked this pull request as ready for review February 21, 2023 01:21
@wadepickett
Copy link
Contributor Author

wadepickett commented Feb 21, 2023 •
edited
Loading

@Rick-Anderson, I did not put the note in all advanced topics, which seemed too broad. I put the note in the following topics that seemed like a good place and context to remind the target audience who might be updating their existing libraries. :

  • signalr/api-design.md
  • signalr/configuration.md (Moved latest version to top)
  • signalr/hub-filters.md
  • signalr/hubcontext.md
  • signalr/hubs.md (Moved latest version to top)
  • signalr/streaming.md

I specified ASP.NET Core SignalR where otherwise I would be adding a new version moniker for a few lines of text. I felt like adding more little moniker chunks in some of those old topics where there were no moniker versioning to begin with was going to make it needlessly messy for future updates.

There were 2 topics where I needed to pull the latest version to the top. So you will have to search for the phrase "SignalR no longer depends" to see what I added in those. Normally we would move versions to the top first, merge and then edit so that they would be cleaner, but they were being added afterwards.

@wadepickett
Copy link
Contributor Author

...adding additional review links...

Rick-Anderson
Rick-Anderson suggested changes Feb 21, 2023
View reviewed changes
@wadepickett wadepickett marked this pull request as draft February 21, 2023 02:02
@Rick-Anderson
Copy link
Contributor

Dan Roth likes to avoid history lessons. I'd be inclined to write it

ASP.NET Core SignalR doesn't depend on getting many assemblies from NuGet. For more information, see "

Can you provide a link to the relevant migration doc?
As I previously stated, This only needs to be in a couple places and probably doesn't belong in onboarding docs. Folks migrating:

  1. Don't look in on boarding docs.
  2. Look in the migration docs.

@wadepickett
Copy link
Contributor Author

wadepickett commented Feb 21, 2023 •
edited
Loading

I did provide a link to the migration doc. Maybe I don't understand what you are referring to when you say "the migration doc".

As far as I saw, the only thing we had that we referred to as "the migration" doc for signal R was the signalr/version-differences.md doc. In #6463 you ask for a new migration doc, Brennan says to use the signalr/version-differences.md, he updates that then that is "the migrations" doc for SignalR. That is what I have been linking to since you asked to link to the migrations doc instead of what Brennan suggested to use. I linked to here: For more information, see Differences between ASP.NET SignalR and ASP.NET Core SignalR. That seemed to make sense since that topic covers the differences in where the libraries are between versions, right at the top.

You must mean the general migration topics instead if not the signalr/version-differences.md you all decided fit the bill as the SignalR migrations doc.

2.2 to 3.0 is the only one that even mentions signalR, but nothing on this topic. I can add a new section to Migration 2.2 to 3.0 on SignalR for this topic if that is what you were thinking.

@Rick-Anderson
Copy link
Contributor

2.2 to 3.0 is the only one that even mentions signalR, but nothing on this topic. I can add a new section to Migration 2.2 to 3.0 on SignalR for this topic if that is what you were thinking.

Exactly. That's where it belongs. Put a couple links to that in the more resources section of a couple docs.

@wadepickett wadepickett marked this pull request as ready for review February 22, 2023 09:13
@wadepickett
Copy link
Contributor Author

wadepickett commented Feb 22, 2023 •
edited
Loading

@Rick-Anderson,

  • I moved the note into the include as you suggested.

  • I added a small new linkable sub section to the end of the SignalR section of the migration docs.

  • I'll assume you still want the Note in the hubs.md topic Brennan called out originally, so it stands out enough to take care of the problem Brennan was concerned with. Then you said you want "links to [the new migration section] in the more resources section of a couple docs." That sounds like the couple other docs would only have the more resources link, which I provided.

  • I added the resource link in the following topics that seemed relevant, (but it is hard for me to tell where you would draw the line at an "onboarding" level on these so hopefully these are right):

  • I backed out of the other topics.

Thanks for the suggestions.

BrennanConroy
BrennanConroy reviewed Feb 22, 2023
View reviewed changes
Rick-Anderson
Rick-Anderson reviewed Feb 22, 2023
View reviewed changes
@Rick-Anderson
Copy link
Contributor

ASP.NET Core SignalR server-side assemblies are now installed with the .NET Core SDK. For more information see This link in the migration doc.

@wadepickett
Copy link
Contributor Author

ASP.NET Core SignalR server-side assemblies are now installed with the .NET Core SDK. For more information see This link in the migration doc.

OK, thanks. Reducing the new section in the migration topic to just that.

Rick-Anderson
Rick-Anderson approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@Rick-Anderson Rick-Anderson left a comment

Choose a reason for hiding this comment

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

OK with me as long as @BrennanConroy approves

BrennanConroy
BrennanConroy reviewed Feb 23, 2023
View reviewed changes
wadepickett
wadepickett commented Feb 24, 2023
View reviewed changes
wadepickett and others added 2 commits February 24, 2023 10:58
@wadepickett wadepickett merged commit 36beb3e into main Feb 24, 2023
@wadepickett wadepickett deleted the wadepickett/27325SignalRrefSharedFramework branch February 24, 2023 20:23
Donciavas pushed a commit to Donciavas/AspNetCore.Docs that referenced this pull request Feb 7, 2024
@wadepickett @Rick-Anderson @Donciavas
SignalR: Reference shared framework (dotnet#28398)
fc14101 
* SignalR: Reference shared framework

* Update aspnetcore/signalr/hubs.md

Committing the inline suggestion, and then will update the missing link afterwards.

Co-authored-by: Rick Anderson <[email protected]>

* Moved 7.0 version up, moved new note to bottom of topics

* Removed dupe.

* added note in 5 more topics, moved latest versions to top where needed

* New include, reduced number of topics, added resources links

* Moved link in migration doc to internal.

* Added description for xref since subsection title not generated

* Removed note from hubcontext, using additional resources link now

* Reduced new section in migration topic per Rick suggestion

* Apply suggestions from code review

Added additional suggestions to link.

Co-authored-by: Rick Anderson <[email protected]>

* Moved note back to near top of hubs.md all 3 versions per Brennan suggestion

---------

Co-authored-by: Rick Anderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rick-Anderson Rick-Anderson Rick-Anderson approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

Assignees

@wadepickett wadepickett

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reference shared framework in SignalR docs
3 participants
@wadepickett @Rick-Anderson @BrennanConroy