Address remaining Blazor integration PR feedback#17384
Conversation
0fa6022 to
910da9e
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17384Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17384" |
9b4238d to
4be1b69
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors how the Blazor gateway generates the WASM client configuration response, aiming to produce deployer-configurable manifest expressions while keeping dev-time behavior working, and also improves gateway publish/build inputs (image tag stamping, file locations).
Changes:
- Replace the per-app
IValueProviderconfig generator with aReferenceExpression-based JSON builder using a gateway-origin placeholder. - Improve manifest processing robustness (explicit deserialization failure) and make the
index.htmlroute match case-insensitive. - Stamp/control the .NET Docker image tag via assembly metadata and adjust where gateway/publish artifacts are written/copied.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.Blazor/Scripts/Gateway.cs.in | Tighten nullability for ConfigResponse emission in the gateway script. |
| src/Aspire.Hosting.Blazor/Manifests/EndpointsManifestTransformer.cs | Add explicit deserialize failure and case-insensitive index.html detection. |
| src/Aspire.Hosting.Blazor/GatewayOriginReference.cs | New helper to emit a deployer-resolvable origin placeholder while resolving dev-time values. |
| src/Aspire.Hosting.Blazor/GatewayConfigurationBuilder.cs | Major refactor: build config JSON as a ReferenceExpression and remove logger-based warning path. |
| src/Aspire.Hosting.Blazor/BlazorHostedExtensions.cs | Adjust call site for the new config builder signature. |
| src/Aspire.Hosting.Blazor/BlazorGatewayExtensions.cs | Add [ResourceName], change artifact paths, and revise .NET image tag resolution logic. |
| src/Aspire.Hosting.Blazor/Aspire.Hosting.Blazor.csproj | Add BlazorGatewayDotNetImageTag property and stamp it into assembly metadata. |
| playground/BlazorStandalone/README.md | Documentation wording update (“Aspire” instead of “.NET Aspire”). |
| playground/BlazorHosted/README.md | Documentation wording update (“Aspire” instead of “.NET Aspire”). |
PR feedback from microsoft#15691: - Add [ResourceName] attribute to all Add* API name parameters - Replace null-forgiving operator with explicit throw in EndpointsManifestTransformer - Use StringComparison.OrdinalIgnoreCase for route comparison - Fix '.NET Aspire' phrasing in README files Docker Compose publish fixes for Blazor gateway: - Add GatewayOriginReference (IValueProvider + IManifestExpressionProvider) - Use ReferenceExpression for ConfigResponse so publishers emit proper placeholders - Use TextEncoderSettings to escape braces in JSON string values - Simplify Gateway.cs.in (serve ConfigResponse directly, no Replace logic) - Add System.Text.Unicode dependency for brace-escaping encoder Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4be1b69 to
ae9ce87
Compare
JamesNK
left a comment
There was a problem hiding this comment.
One issue found: missing OTLP misconfiguration warning in the hosted Blazor path (BlazorHostedExtensions.cs). The rest of the changes look correct.
| @@ -99,7 +99,6 @@ private static void EnsureEnvironmentCallback( | |||
| annotation.Services, | |||
There was a problem hiding this comment.
The OTLP misconfiguration warning was re-added at the call site in BlazorGatewayExtensions.cs (standalone gateway path), but the equivalent warning is missing here in the hosted Blazor path. When httpOtlpEndpointUrl is null and annotation.ProxyBlazorTelemetry is true, the user gets no diagnostic indicating that WASM client telemetry won't be forwarded.
The old ClientConfigValueProvider.LogOtlpWarningIfNeeded() covered both paths; now only the standalone gateway warns.
var httpOtlpEndpointUrl = BlazorGatewayExtensions.ResolveHttpOtlpEndpointUrl(context, host.ApplicationBuilder.Configuration);
if (httpOtlpEndpointUrl is null && annotation.ProxyBlazorTelemetry)
{
context.Logger.LogWarning(
"OTLP telemetry proxying was requested but no dashboard HTTP endpoint could be resolved. " +
"WASM client telemetry will not be forwarded.");
}There was a problem hiding this comment.
Ensure there are tests for both scenarios that check a warning is logged.
There was a problem hiding this comment.
Done — added in 9759ffb:
- \ProxyTelemetry_LogsWarning_WhenOtlpEndpointNotResolvable\ — verifies the warning fires when ProxyBlazorTelemetry is true but no HTTP OTLP endpoint is available.
- \ProxyTelemetry_DoesNotLogWarning_WhenOtlpEndpointIsConfigured\ — verifies no spurious warning when the endpoint is properly configured.
There was a problem hiding this comment.
Added the equivalent warning in the hosted path in 9759ffb. Both standalone and hosted paths now log when telemetry proxying is requested but no OTLP endpoint is resolvable.
| var format = json | ||
| .Replace("{", "{{") | ||
| .Replace("}", "}}") | ||
| .Replace(OriginToken, "{0}"); |
| // Send only the OTLP path so the WASM client resolves it against its own | ||
| // page origin (HostEnvironment.BaseAddress). This avoids cross-origin issues | ||
| // when the user navigates via HTTP but the gateway also exposes HTTPS. | ||
| environment["ASPIRE_OTLP_PATH_BASE"] = $"{pathBase}/{otlpPrefix}"; |
There was a problem hiding this comment.
I didn't notice before, but this basically replaces OTEL_EXPLORTER_OTLP_ENDPOINT.
I would prefer that the standard endpoint env var was still used, and then clients replaced the scheme/host with the current web page scheme/host.
There was a problem hiding this comment.
Done — renamed ASPIRE_OTLP_PATH_BASE to OTEL_EXPORTER_OTLP_ENDPOINT in 3832dea. The value remains a relative path that the WASM client resolves against its page origin (HostEnvironment.BaseAddress) to stay same-origin and avoid CORS issues.
The standalone gateway path already logged a warning when telemetry proxying was requested but no HTTP OTLP endpoint could be resolved. The hosted model (ProxyBlazorTelemetry via BlazorHostedExtensions) was missing the equivalent diagnostic. - Add LogWarning in BlazorHostedExtensions.EnsureEnvironmentCallback - Add tests verifying the warning fires (and does not fire) in both scenarios Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the standard OpenTelemetry environment variable name instead of a custom Aspire-specific one. The value remains a relative path that the WASM client resolves against its page origin to stay same-origin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Addresses remaining unresolved review threads from #15691 and adds Docker Compose publish support for the Blazor gateway.
PR Feedback Fixes
[ResourceName]attribute toAddBlazorGateway,AddBlazorWasmProject<T>, andAddBlazorWasmAppname parameters?? throwinEndpointsManifestTransformerStringComparison.OrdinalIgnoreCasefor route comparison"Aspire:Store:Path"magic string to a constDocker Compose Publish Support
The
ClientConfigValueProviderclass (IValueProvider + IManifestExpressionProvider) is replaced with a staticBuildConfigExpression()method that returns aReferenceExpression. This makes the ConfigResponse environment variable natively understood by all publishers.How it works:
__ORIGIN__token in place of the gateway URLUnsafeRelaxedJsonEscaping(we control all values, no braces present){->{{) forstring.Formatsafety__ORIGIN__with{0}to create the format stringReferenceExpressionwithGatewayOriginReferenceas the argumentResult:
https://localhost:PORT— ConfigResponse contains absolute URLs$${GATEWAY_BINDINGS_HTTPS_URL}— deployer fills.envNew file:
GatewayOriginReference.cs— lightweight IValueProvider + IManifestExpressionProvider that wraps an EndpointReference, trims trailing slashes, and emits manifest expressions for publishers.