search

dotnet-presentations / blazor-workshop

Blazor workshop
https://aka.ms/blazorworkshop
MIT License
3.51k stars 1.56k forks source link

Use SignalR instead of client-side polling #322

Closed IEvangelist closed 3 years ago

IEvangelist commented 3 years ago

Note

This pull request is targeting a newly created signalr branch, that I plan on using for a new SignalR Learn module.

Summary of proposed changes in this PR:

Intended code for new Microsoft Learn module, also relates to https://github.com/dotnet/AspNetCore.Docs/issues/23357

/cc @bradygaster @danroth27 @SteveSandersonMS

SteveSandersonMS commented 3 years ago

Thanks for suggesting these changes, @IEvangelist!

Do you have a sense of what impact these changes would have on all the written instructions for the various chapters under docs? I think the main determinant of whether we want to make a given design change to the code is what effect it has on the understandability of the steps people have to follow under docs (secondarily, a goal would be minimizing the concept count of the workshop, at least concepts not directly central to Blazor).

To merge the PR we'd need all the docs and all the save-points to be correspondingly updated. But that's a lot of work so before you get too deep into it, I'd recommend working out an overview of how impactful this would be on the docs. Ideally we'd be reducing the number of non-Blazor-specific steps they are carrying out, and the amount of non-Blazor-specific code concepts.

SteveSandersonMS commented 3 years ago

Oh wait, it looks like you're not aiming to merge into master, but rather are creating a different branch. You might want to delete all the Blazor-related docs and save-points from your signalr branch since they don't seem relevant to it. Not sure what your longer term plan is for the signalr branch and to what extent you intend to maintain it.

IEvangelist commented 3 years ago

Thanks for suggesting these changes, @IEvangelist!

Do you have a sense of what impact these changes would have on all the written instructions for the various chapters under docs? I think the main determinant of whether we want to make a given design change to the code is what effect it has on the understandability of the steps people have to follow under docs (secondarily, a goal would be minimizing the concept count of the workshop, at least concepts not directly central to Blazor).

To merge the PR we'd need all the docs and all the save-points to be correspondingly updated. But that's a lot of work so before you get too deep into it, I'd recommend working out an overview of how impactful this would be on the docs. Ideally we'd be reducing the number of non-Blazor-specific steps they are carrying out, and the amount of non-Blazor-specific code concepts.

Hi @SteveSandersonMS,

The idea was to have this be in its own branch, specific to signalr as an alternative to the client-side polling. I do not plan on having it impact any of the docs or existing instructions. After a few discussions it was decided that this was the cleanest approach for this. We want to keep the Learn module focused on Pizza APIs. Does that make sense, or would you propose an alternate approach?

SteveSandersonMS commented 3 years ago

Right yes, I just spotted that. If this doesn't affect master then you can basically do what you like 😄

IEvangelist commented 3 years ago

Right yes, I just spotted that. If this doesn't affect master then you can basically do what you like 😄

Sounds good, I still value your thoughts on the changes though. Would love a formal review.

bradygaster commented 3 years ago

Whilst I like any addition of SignalR and feel this does reflect a nice usage of "all our stuff" together, I'd question having forks in the docs unless you're thinking of eventually having something like a pivot in the doc. Options tend to confuse folks, so I want to make sure we're providing the RIGHT option.

@SteveSandersonMS do you have any objection to the actual implementation @IEvangelist is going with here? Is it more a concern of the implication on the docs and other content pieces or, is this a detraction from The Blazor Way (TM) and if so, is there a compromise?

SteveSandersonMS commented 3 years ago

@bradygaster We designed the blazor-workshop to teach people Blazor. To give people the best chance of really learning this stuff in a 2-day in-person workshop, it needs that focus. Even Blazor alone these days contains more than you can fully cover in 2 days, so minimizing the amount of adjacent concepts from ASP.NET Core is important.

My concern with including this in master is just that I don't know from this PR what impact it has on the workshop flow through the docs, because those changes aren't in this PR (and I know it will be a significant effort for @IEvangelist to go through every step in docs to update them and all the save-points). If it has no impact at all on docs, then in theory I'd be fine with SignalR/BackgroundService becoming some new implementation detail. However personally I'm a bit too focused on .NET 6 RC2/GA right now to dig too deeply into the new changes here and really investigate if there are drawbacks like potential bugs or security issues in any of the new code. I hope that sounds reasonable!

IEvangelist commented 3 years ago

Hi again @SteveSandersonMS and @bradygaster,

To be clear, I wasn't planning on targeting master. That is why we have a new branch named signalr, and I plan on having my SignalR Learn module point there instead. I'll make it clear that it's an adaptation of the existing Pizza app, focusing on refactoring the client-side polling into using SignalR. Perhaps depending on how well the module does, then we could reevaluate its value, and I could then propose updates to the docs to formally introduce the concepts here - post .NET 6.

Does that sound good?

SteveSandersonMS commented 3 years ago

@IEvangelist Using a separate branch is fine with me, like mentioned before.

IEvangelist commented 3 years ago

@IEvangelist Using a separate branch is fine with me, like mentioned before.

@SteveSandersonMS - Perfect, just need an approval to merge. Thanks