-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Javiercn/blazor improved circuit is #11376
Changes from all commits
c33492e
cc7c003
f3934c9
99d05f7
e6e876a
976c67d
8339103
e76d84f
6fcd6d7
4e912a4
9c09ebd
3110646
dd48c5f
fac955c
59a1d69
3a40ef9
0fe9dce
ddb9587
6d4fe51
d921990
62af92e
09870c5
3561ae1
eab5bde
e8efebc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import { discoverPrerenderedCircuits, startCircuit } from './Platform/Circuits/C | |
type SignalRBuilder = (builder: signalR.HubConnectionBuilder) => void; | ||
interface BlazorOptions { | ||
configureSignalR: SignalRBuilder; | ||
serviceUrl: string; | ||
logLevel: LogLevel; | ||
} | ||
|
||
|
@@ -29,6 +30,7 @@ async function boot(userOptions?: Partial<BlazorOptions>): Promise<void> { | |
|
||
const defaultOptions: BlazorOptions = { | ||
configureSignalR: (_) => { }, | ||
serviceUrl: '_blazor', | ||
logLevel: LogLevel.Warning, | ||
}; | ||
|
||
|
@@ -51,9 +53,15 @@ async function boot(userOptions?: Partial<BlazorOptions>): Promise<void> { | |
}); | ||
|
||
// pass options.configureSignalR to configure the signalR.HubConnectionBuilder | ||
const initialConnection = await initializeConnection(options, circuitHandlers, logger); | ||
|
||
const circuits = discoverPrerenderedCircuits(document); | ||
if (circuits.length > 1){ | ||
throw new Error('Can\'t have multiple circuits per connection'); | ||
} | ||
|
||
const circuitId = circuits.length > 0 ? circuits[0].circuitId : await getNewCircuitId(options); | ||
|
||
const initialConnection = await initializeConnection(options, circuitHandlers, circuitId, logger); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Further cleanup can be done to this file, but I'm not going to do it in this PR |
||
for (let i = 0; i < circuits.length; i++) { | ||
const circuit = circuits[i]; | ||
for (let j = 0; j < circuit.components.length; j++) { | ||
|
@@ -65,18 +73,12 @@ async function boot(userOptions?: Partial<BlazorOptions>): Promise<void> { | |
// Ensure any embedded resources have been loaded before starting the app | ||
await embeddedResourcesPromise; | ||
|
||
const circuit = await startCircuit(initialConnection); | ||
|
||
if (!circuit) { | ||
logger.log(LogLevel.Information, 'No preregistered components to render.'); | ||
} | ||
|
||
const reconnect = async (existingConnection?: signalR.HubConnection): Promise<boolean> => { | ||
if (renderingFailed) { | ||
// We can't reconnect after a failure, so exit early. | ||
return false; | ||
} | ||
const reconnection = existingConnection || await initializeConnection(options, circuitHandlers, logger); | ||
const reconnection = existingConnection || await initializeConnection(options, circuitHandlers, circuitId, logger); | ||
const results = await Promise.all(circuits.map(circuit => circuit.reconnect(reconnection))); | ||
|
||
if (reconnectionFailed(results)) { | ||
|
@@ -89,13 +91,15 @@ async function boot(userOptions?: Partial<BlazorOptions>): Promise<void> { | |
|
||
window['Blazor'].reconnect = reconnect; | ||
|
||
const reconnectTask = reconnect(initialConnection); | ||
await reconnect(initialConnection); | ||
|
||
if (circuit) { | ||
circuits.push(circuit); | ||
} | ||
// We render any additional component after all prerendered components have | ||
// re-stablished the connection with the circuit. | ||
const renderedComponents = await startCircuit(circuitId, initialConnection); | ||
|
||
await reconnectTask; | ||
if (!renderedComponents) { | ||
logger.log(LogLevel.Information, 'No preregistered components to render.'); | ||
} | ||
SteveSandersonMS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
logger.log(LogLevel.Information, 'Blazor server-side application started.'); | ||
|
||
|
@@ -104,13 +108,23 @@ async function boot(userOptions?: Partial<BlazorOptions>): Promise<void> { | |
} | ||
} | ||
|
||
async function initializeConnection(options: Required<BlazorOptions>, circuitHandlers: CircuitHandler[], logger: ILogger): Promise<signalR.HubConnection> { | ||
async function getNewCircuitId(options: BlazorOptions): Promise<string> { | ||
const response = await fetch(`${options.serviceUrl}/start`, { | ||
method: 'POST', | ||
}); | ||
|
||
const responseBody = await response.json() as { id: string }; | ||
|
||
return responseBody.id; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the TS-side code, could we rename all references to "circuit ID" to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough |
||
} | ||
|
||
async function initializeConnection(options: Required<BlazorOptions>, circuitHandlers: CircuitHandler[], circuitId: string, logger: ILogger): Promise<signalR.HubConnection> { | ||
|
||
const hubProtocol = new MessagePackHubProtocol(); | ||
(hubProtocol as unknown as { name: string }).name = 'blazorpack'; | ||
|
||
const connectionBuilder = new signalR.HubConnectionBuilder() | ||
.withUrl('_blazor') | ||
.withUrl(`${options.serviceUrl}?circuitId=${circuitId}`) | ||
.withHubProtocol(hubProtocol); | ||
|
||
options.configureSignalR(connectionBuilder); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,8 @@ public static partial class ComponentEndpointRouteBuilderExtensions | |
{ | ||
public static Microsoft.AspNetCore.Components.Server.ComponentEndpointConventionBuilder MapBlazorHub(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints) { throw null; } | ||
public static Microsoft.AspNetCore.Components.Server.ComponentEndpointConventionBuilder MapBlazorHub(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, System.Action<Microsoft.AspNetCore.Http.Connections.HttpConnectionDispatcherOptions> configureOptions) { throw null; } | ||
public static Microsoft.AspNetCore.Components.Server.ComponentEndpointConventionBuilder MapBlazorHub(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, string path) { throw null; } | ||
public static Microsoft.AspNetCore.Components.Server.ComponentEndpointConventionBuilder MapBlazorHub(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, string path, System.Action<Microsoft.AspNetCore.Http.Connections.HttpConnectionDispatcherOptions> configureOptions) { throw null; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an API review point, we should consider wrapping both the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. I would also hope that at some point this ends up being something on ComponentEndpointConventionBuilder instead of overloads. |
||
public static Microsoft.AspNetCore.Components.Server.ComponentEndpointConventionBuilder MapBlazorHub(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, System.Type type, string selector) { throw null; } | ||
public static Microsoft.AspNetCore.Components.Server.ComponentEndpointConventionBuilder MapBlazorHub(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, System.Type type, string selector, System.Action<Microsoft.AspNetCore.Http.Connections.HttpConnectionDispatcherOptions> configureOptions) { throw null; } | ||
public static Microsoft.AspNetCore.Components.Server.ComponentEndpointConventionBuilder MapBlazorHub(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, System.Type componentType, string selector, string path) { throw null; } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using Microsoft.AspNetCore.Authorization; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line probably isn't required on its own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, this is cleanup. |
||
using Microsoft.AspNetCore.DataProtection; | ||
|
||
namespace Microsoft.AspNetCore.Components.Server | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,7 @@ public static void SetCurrentCircuitHost(CircuitHost circuitHost) | |
public event UnhandledExceptionEventHandler UnhandledException; | ||
|
||
public CircuitHost( | ||
string circuitId, | ||
CircuitId circuitId, | ||
IServiceScope scope, | ||
CircuitClientProxy client, | ||
RendererRegistry rendererRegistry, | ||
|
@@ -80,7 +80,7 @@ public CircuitHost( | |
Renderer.UnhandledSynchronizationException += SynchronizationContext_UnhandledException; | ||
} | ||
|
||
public string CircuitId { get; } | ||
public CircuitId CircuitId { get; } | ||
|
||
public Circuit Circuit { get; } | ||
|
||
|
@@ -140,7 +140,7 @@ internal void SendPendingBatches() | |
var _ = Renderer.InvokeAsync(() => Renderer.ProcessBufferedRenderBatches()); | ||
} | ||
|
||
public async Task InitializeAsync(CancellationToken cancellationToken) | ||
public async Task InitializeAsync(CancellationToken cancellationToken, bool existingCircuit) | ||
{ | ||
await Renderer.InvokeAsync(async () => | ||
{ | ||
|
@@ -149,8 +149,15 @@ await Renderer.InvokeAsync(async () => | |
SetCurrentCircuitHost(this); | ||
_initialized = true; // We're ready to accept incoming JSInterop calls from here on | ||
|
||
await OnCircuitOpenedAsync(cancellationToken); | ||
await OnConnectionUpAsync(cancellationToken); | ||
if (!existingCircuit) | ||
{ | ||
// If the circuit we are using was created during prerendering don't run | ||
// the lifecycle events again at this time. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this a bug before? I'm surprised we need to change that behavior unless it was a bug before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit uncertain about the meanings of these lifecycle events now. Previously I would have expected:
... but it seems with this change that there wouldn't be a way to know when the socket connection was opened. Is this right? I'm not sure we've got a formal definition of when these lifecycle events should fire, but now would be a good time to be explicit about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way things work now: Prerendered components or prerendered+non-prerendered
Only non prerendered components
OnConnectionUp always marks when the socket is opened and we're ready to perform JSInterop. |
||
// Circuit opened ran during prerendering | ||
// ConnectionUp will run after the reconnection is completed. | ||
await OnCircuitOpenedAsync(cancellationToken); | ||
await OnConnectionUpAsync(cancellationToken); | ||
} | ||
|
||
// We add the root components *after* the circuit is flagged as open. | ||
// That's because AddComponentAsync waits for quiescence, which can take | ||
|
@@ -289,7 +296,7 @@ private async Task OnCircuitDownAsync() | |
|
||
public async ValueTask DisposeAsync() | ||
{ | ||
Log.DisposingCircuit(_logger, CircuitId); | ||
Log.DisposingCircuit(_logger, CircuitId.RequestToken); | ||
|
||
await Renderer.InvokeAsync(async () => | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included here so that its easier to test with the rest of the repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is merged, I'm guessing you'll remove this, and ensure the CTI scripts give us adequate coverage of the SignalR Service scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't hurt CTI, it only puts the reference in our build infrastructure so that we can do
<Reference Import="Microsoft.Azure.SignalR" />
in our test projects when we want to test something against Azure SignalRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be constantly broken while we're still making changes. I'd suggest not doing this right now and wait until we stop making big API changes that break the azure signalr SDK.