[Blazor] Manual reflection based page discovery #49757

javiercn · 2023-07-31T15:45:52Z

Addresses [Blazor] Component application discovery APIs #49756, [Blazor] Startup APIs API review #49754
Incorporates the API review fixes.
Addresses [Blazor] WebAssemblyComponentsEndpointOptions.cs should be defined in AddWebAssemblyRenderMode not in the DI container #49753
- There are small changes to the public API here that need to go through API review

halter73 · 2023-08-03T19:24:11Z

src/Components/Endpoints/src/Builder/RazorComponentEndpointConventionBuilderExtensions.cs

+    /// <remarks>
+    /// The provided assemblies will be scanned for pages that will be mapped as endpoints.
+    /// </remarks>
+    public static RazorComponentEndpointConventionBuilder AddAssemblies(


Did we settle on AddAssemblies? That's my preference and what I thought we agreed on, but you were sharing your screen, and your most recent edit of the proposal #49756 showed AddAdditionalAssemblies. I can update #49756 (comment). I just want to make sure this is intentional.

The thing I like about AddAdditionalAssemblies is that the name implies that not all assemblies have to be added here, just the "additional" ones, i.e., external assemblies that don't get scanned by default.

I don't understand that implication at all. We don't call AddSingleton AddAdditionalSingleton just because there are some default singleton services like IServiceProvider.

That said, I said basically this during API review, and I think the consensus was that we should add Additional to the method name to make it super clear that the main assembly is included by default.

src/Components/Endpoints/src/Builder/RazorComponentEndpointConventionBuilder.cs

halter73 · 2023-08-03T19:33:02Z

...bAssembly/Server/src/Builder/WebAssemblyRazorComponentEndpointConventionBuilderExtensions.cs

+    /// <returns>The <see cref="RazorComponentEndpointConventionBuilder"/>.</returns>
+    public static RazorComponentEndpointConventionBuilder AddWebAssemblyRenderMode(
+        this RazorComponentEndpointConventionBuilder builder,
+        WebAssemblyComponentsEndpointOptions? options = null)


This is another one where I personally prefer WebAssemblyComponentsEndpointOptions, but we approved WebAssemblyComponentsEndpointsOptions in the API approval.

Agreed - I've just changed it to WebAssemblyComponentsEndpointOptions

src/Components/WebAssembly/Server/src/PublicAPI.Unshipped.txt

src/Components/Endpoints/src/DependencyInjection/RazorComponentOptions.cs

src/Components/Endpoints/src/PublicAPI.Unshipped.txt

halter73 · 2023-08-03T19:42:11Z

Do we expect the RenderModeEndpointProvider namespace change we approved in #49756 in another PR?

MackinnonBuck · 2023-08-11T17:28:16Z

I've just addressed some of the PR feedback about API changes. This is ready to be reviewed again.

halter73

I'm going to approve to unblock, but we should update the API proposal to match what we're doing, and have a discussion about the major changes to AddWebAssemblyRenderMode.

halter73 · 2023-08-11T18:50:43Z

src/Components/Endpoints/src/Builder/RazorComponentsEndpointConventionBuilder.cs

+    /// Adds the given <paramref name="renderMode"/> to the list of configured render modes if not present.
+    /// </summary>
+    /// <param name="renderMode">The <see cref="IComponentRenderMode"/> to add.</param>
+    public void AddRenderMode(IComponentRenderMode renderMode)


Suggested change

public void AddRenderMode(IComponentRenderMode renderMode)

public RazorComponentsEndpointConventionBuilder AddRenderMode(IComponentRenderMode renderMode)

Wasn't there supposed to be AddWebAssemblyRenderMode() directly on the RazorComponentsEndpointConventionBuilder according to the approved API proposal at #49754?

I don't think we approved any IComponentRenderMode APIs other than RenderModeEndpointProvider which we tried to hide in an Infrastructure namespace. I don't think RenderMode or the IComponentRenderMode interface itself was ever proposed as API either.

I see that this is being called by an AddWebAssemblyRenderMode extension method in WebAssemblyRazorComponentsEndpointConventionBuilderExtensions. I'm assuming this was done for layering reasons so we could flow options via the call to AddWebAssemblyRenderMode(WebAssemblyComponentsEndpointOptions).

It's really odd to use different patterns for adding server and webassembly render modes. If we're going to do an extension method for one, we should do it for both. It's also a bit unusual to take an WebAssemblyComponentsEndpointOptions directly instead of an Action<WebAssemblyComponentsEndpointOptions>, but maybe it's okay since we're not really doing the options pattern for endpoint config. Regardless, it's something that should be discussed in API review.

src/Components/WebAssembly/Server/src/Builder/WebAssemblyRenderModeWithOptions.cs

src/Components/test/testassets/Components.TestServer/RazorComponentEndpointsStartup.cs

halter73 · 2023-08-11T18:58:20Z

src/Components/test/testassets/Components.TestServer/RazorComponentEndpointsStartup.cs

-                    .AddWebAssemblyRenderMode();
+                    .AddWebAssemblyRenderMode(new WebAssemblyComponentsEndpointOptions
+                    {
+                        PathPrefix = "/WasmMinimal"


AddWebAssemblyComponents is the much more natural place to configure options since it follows the options pattern and aligns nicely with AddServerComponents.

If we're going to do this last-minute non-API-approved change, shouldn't we at least test what happens if you need per-MapRazorComponents WebAssmblyComponentsEndpointOptions.PathPrefix configuration? And verify everything works if you call MapRazorComponents multiple times with different path prefixes? This is the kind of thing that often doesn't work if you don't test it. I had to do extra work to make MapIdentityApi<TUser> work if you called it with multiple different TUser types in one application that I would have never found without testing.

If this is something that is so difficult and unusual that we cannot even test it in time, we're better off going with the simpler global option we had before. If we need per-MapRazorComponents of this in the future, we could always add a global override that works exactly like this.

I just tested this locally and it works. IMO, making this an option on the endpoint is ideal because hosting multiple WebAssembly apps is something we already support and document, and ideally we'd want an equivalent pattern with these new APIs.

I can see that there's still some deviation from the original API proposal with this PR, so I'm going to update the API review issue(s) to match these changes and we can make a final decision on Monday.

I'm glad it works. I'm fine with keeping it. We should probably make AddServerRenderMode an extension method as well for consistency. But we can merge this as-is for now and make any final adjustments after we API review it.

And can we add an automated test for this too then?

And can we add an automated test for this too then?

For sure. Is it fine if we do that in a follow-up? It's actually a bit involved, and I probably won't be able to finish by EOD.

That's fine with me.

eerhardt · 2023-08-29T18:56:42Z

src/Components/Endpoints/src/DependencyInjection/RazorComponentsServiceCollectionExtensions.cs

    /// <returns>An <see cref="IRazorComponentsBuilder"/> that can be used to further configure the Razor component services.</returns>
-    [RequiresUnreferencedCode("Razor Components does not currently support trimming or native AOT.", Url = "https://aka.ms/aspnet/trimming")]
-    public static IRazorComponentsBuilder AddRazorComponents(this IServiceCollection services)
+    [RequiresUnreferencedCode("Razor Components does not currently support trimming or native AOT.", Url = "https://aka.ms/aspnet/nativeaot")]


Why was this URL changed? I believe the intention in #49377 was for this URL to be https://aka.ms/aspnet/trimming.

ghost added the area-blazor Includes: Blazor, Razor Components label Jul 31, 2023

javiercn force-pushed the javiercn/reflection-based-component-discovery branch from 159a617 to 264804c Compare August 3, 2023 11:56

javiercn marked this pull request as ready for review August 3, 2023 12:11

javiercn requested review from a team as code owners August 3, 2023 12:11

javiercn force-pushed the javiercn/reflection-based-component-discovery branch from 0f932bd to fbd01d0 Compare August 3, 2023 12:57

javiercn added 12 commits August 3, 2023 18:57

Support configuring hub options

f2c7dd9

tmp

5761a60

make build pass

1a1ce6e

Remove unnecessary APIs

33ea24c

Remove more APIs

c86bd52

Apply API review feedback

7e07491

More feedback, fix buid, etc.

1334386

API updates and fixes

bd441b3

Add WasmMinimal

d81dd4b

Undo API change

5eb13cf

Fix build

32f4811

Fix build

637d7ef

javiercn force-pushed the javiercn/reflection-based-component-discovery branch from 54723f6 to 637d7ef Compare August 3, 2023 17:16

halter73 reviewed Aug 3, 2023

View reviewed changes

MackinnonBuck added 2 commits August 11, 2023 09:24

Merge branch 'main' into javiercn/reflection-based-component-discovery

e9bf108

API changes

8987928

MackinnonBuck requested a review from halter73 August 11, 2023 17:27

halter73 approved these changes Aug 11, 2023

View reviewed changes

PR feedback

39c6214

MackinnonBuck mentioned this pull request Aug 11, 2023

[Blazor] Component application discovery APIs #49756

Closed

MackinnonBuck enabled auto-merge (squash) August 11, 2023 23:37

MackinnonBuck merged commit 4802de1 into main Aug 12, 2023

MackinnonBuck deleted the javiercn/reflection-based-component-discovery branch August 12, 2023 01:42

ghost added this to the 8.0-rc1 milestone Aug 12, 2023

danroth27 mentioned this pull request Aug 16, 2023

Referenced pages are not found #50100

Closed

1 task

eerhardt reviewed Aug 29, 2023

View reviewed changes

danroth27 mentioned this pull request Aug 30, 2023

Unable to route to Blazor component pages in the client assembly #49313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Blazor] Manual reflection based page discovery #49757

[Blazor] Manual reflection based page discovery #49757

javiercn commented Jul 31, 2023 •

edited

Loading

halter73 Aug 3, 2023

MackinnonBuck Aug 11, 2023

halter73 Aug 11, 2023

halter73 Aug 3, 2023

MackinnonBuck Aug 11, 2023

halter73 commented Aug 3, 2023

MackinnonBuck commented Aug 11, 2023

halter73 left a comment

halter73 Aug 11, 2023

halter73 Aug 11, 2023 •

edited

Loading

MackinnonBuck Aug 11, 2023 •

edited

Loading

halter73 Aug 11, 2023

halter73 Aug 11, 2023 •

edited

Loading

MackinnonBuck Aug 11, 2023

halter73 Aug 11, 2023

eerhardt Aug 29, 2023

	public void AddRenderMode(IComponentRenderMode renderMode)
	public RazorComponentsEndpointConventionBuilder AddRenderMode(IComponentRenderMode renderMode)

[Blazor] Manual reflection based page discovery #49757

[Blazor] Manual reflection based page discovery #49757

Conversation

javiercn commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 commented Aug 3, 2023

MackinnonBuck commented Aug 11, 2023

halter73 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

MackinnonBuck Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn commented Jul 31, 2023 •

edited

Loading

halter73 Aug 11, 2023 •

edited

Loading

MackinnonBuck Aug 11, 2023 •

edited

Loading

halter73 Aug 11, 2023 •

edited

Loading