Skip to content

Use MessagePackReader \ MessagePackWriter to implement IHubProtocol for server-side Blazor #8687

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 3 commits into from
Apr 1, 2019
Merged

Use MessagePackReader \ MessagePackWriter to implement IHubProtocol for server-side Blazor #8687

merged 3 commits into from
Apr 1, 2019

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Mar 20, 2019 •
edited
Loading

Fixes #7311

@pranavkm pranavkm requested a review from BrennanConroy March 20, 2019 20:02
@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 20, 2019
BrennanConroy
BrennanConroy reviewed Mar 25, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Talk to @Eilon about the submodule/code usage

Eilon
Eilon reviewed Mar 28, 2019
View reviewed changes
davidfowl
davidfowl reviewed Mar 29, 2019
View reviewed changes
src/Components/Server/src/Circuits/RemoteRenderer.cs Outdated
renderBatchWriter.Write(in batch);
}

var batchBytes = memoryStream.ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

You can avoid the second array allocation if you supported Memory<byte> or ArraySegment<byte>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be in SignalR?

@pranavkm pranavkm force-pushed the prkrishn/blazorpack branch from f707609 to 006f1a9 Compare March 29, 2019 20:51
@pranavkm pranavkm marked this pull request as ready for review March 29, 2019 20:52
@pranavkm pranavkm force-pushed the prkrishn/blazorpack branch from 006f1a9 to 1f7ee37 Compare March 29, 2019 20:52
@pranavkm
Copy link
Contributor Author

🆙 📅

@Tratcher Tratcher removed their request for review March 29, 2019 23:26
rynowak
rynowak reviewed Mar 30, 2019
View reviewed changes
src/Components/Browser.JS/src/Boot.Server.ts
@@ -42,9 +42,11 @@ async function boot() {
}

async function initializeConnection(circuitHandlers: CircuitHandler[]): Promise<signalR.HubConnection> {
const hubProtocol = new MessagePackHubProtocol();
(hubProtocol as any).name = 'blazorpack';
Copy link
Member

Choose a reason for hiding this comment

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

😻

@pranavkm pranavkm force-pushed the prkrishn/blazorpack branch from 1f7ee37 to 913a8f1 Compare April 1, 2019 18:09
@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 1, 2019

@aspnet-hello triage https://github.com/aspnet/AspNetCore/pull/8687/checks?check_run_id=90265125

@BrennanConroy
Copy link
Member

Wrong link

@BrennanConroy
Copy link
Member

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=139471&view=logs&jobId=2fe95fc5-ae3d-58fd-c2b8-46bb81541875

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact ryanbrandenburg.

I've triaged the above build.

@pranavkm pranavkm merged commit d86c9b3 into master Apr 1, 2019
@pranavkm pranavkm deleted the prkrishn/blazorpack branch April 1, 2019 21:25
@Eilon
Copy link
Member

Eilon commented Apr 1, 2019

Hold on, I didn't sign off on this. We need 3rd Party Notices for this.

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 1, 2019

@Eilon oops. Should I back out the change?

@Eilon
Copy link
Member

Eilon commented Apr 1, 2019

Naah it's fine, I'll let you know what to add.

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

@halter73 halter73 halter73 left review comments

@davidfowl davidfowl davidfowl left review comments

@Eilon Eilon Eilon left review comments

@rynowak rynowak rynowak left review comments

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@analogrelay analogrelay Awaiting requested review from analogrelay

@mikaelm12 mikaelm12 Awaiting requested review from mikaelm12

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pranavkm @BrennanConroy @aspnet-hello @Eilon @halter73 @davidfowl @rynowak