Skip to content
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

Integration article updates #31445

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Integration article updates #31445

merged 1 commit into from
Jan 19, 2024

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Jan 13, 2024

Fixes #31429

Thanks @nitrospaz! 🚀 ... and sorry that you ran into trouble with the guidance.

Touch-ups

I made a few light touch-ups to where setting the namespace for components are called out. These didn't really need to be changed, but I made improvements that show an example of the using/@using statement(s) with a placeholder ({APP NAMESPACE}) and then an example of an actual statement with BlazorSample as the app's namespace (e.g., using BlazorSample.Components).

Bug 1 🪲

In order to use the shortened syntax for render modes, such as ...

@rendermode InteractiveServer

... the _Imports.razor file needs an additional @using ...

@using static Microsoft.AspNetCore.Components.Web.RenderMode

Right at the time of release of the 8.0 framework, they tossed that @using statement into the _Imports.razor file. I went around the repo updating examples to include the line; but unfortunately, I missed this article for that update 😢. Therefore, I'm adding that line now. I'm making a tracking note to confirm that there are no other spots around the repo that need the line.

Bug 2 😈

This one is a little more painful to see, but I understand why it happened. It turns out ... AFAICT† ... that in order to embed an interactive Razor component into a page or view with the Component Tag Helper that the guidance is correct that MapRazorComponents is called with AddInteractiveServerRenderMode chained on the call ...

app.MapRazorComponents<App>()
    .AddInteractiveServerRenderMode();

As you said in the issue, that section doesn't show or explain that the App component must be added to the app (and the namespace for it). The section that follows this section, the one that addresses routable components (i.e., not embedded into a page or view but actually routable by URL request) has the App component, but this section on embedding them doesn't. It only shows MapRazorComponents with the root App component specified.

The interesting thing is that in order to embed components with the Component TH per this section, one doesn't really need a functional App component for that line AFAICT†. The EmbeddedCounter component works following the guidance in this section if the App component is just a no-op file ...

Components/App.razor:

@* No-op App component *@

That brings me to the ...............

AFAICT† ... This didn't come up in conversations with the product unit (PU) when I was working on the coverage. We need to ask Mackinnon what he thinks about a no-op App component. If he thinks it's totally fine in this scenario, namely a RP/MVC app that will only embed non-routable interactive components into pages and views, then we can go ahead with these updates. He may have a known solution for this scenario that doesn't require a no-op App component. If he feels that we need to get more PU 👁️👁️👁️👁️ on this because the idea of a no-op App component seems strange or just flat-out wrong 🙈, then he'll ping some additional folks to look.

The reason that this was missed is that the test app I was using here already had the App component present because I worked up the routable scenario first. Because it was already present, the MapRazorComponents line didn't throw. I'm sorry now that I didn't work this section from a clean MVC app. At that time, I was 🏃 like crazy 😵 to get a massive amount of work done for release. Gremlins 😈 like this are virtually impossible to avoid working that fast.

Because we're on a three-day weekend holiday, we probably won't get a response until next week. Stand-by .............


Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/components/integration.md Integrate ASP.NET Core Razor components into ASP.NET Core apps

@MackinnonBuck
Copy link
Member

the _Imports.razor file needs an additional @using ...

Yep, good catch!

The interesting thing is that in order to embed components with the Component TH per this section, one doesn't really need a functional App component...

Yeah, that's correct. It's a little weird - in the use case demonstrated by these docs, we only care about the side effects of MapRazorComponents that are unrelated to page component routing (e.g., setting up the _framework/blazor.web.js endpoint).

Rather than declaring an empty App component, you could also use the existing _Imports component:

app.MapRazorComponents<_Imports>()
    .AddInteractiveServerRenderMode();

...but that's still pretty odd. I wonder if we should consider adding a non-generic overload of MapRazorComponents() for cases like this. @javiercn, do you have any thoughts about this?

@guardrex
Copy link
Collaborator Author

adding a non-generic overload of MapRazorComponents() for cases like this

Is it OK for the PR to go ahead with the no-op App component approach for now? It sounds like you're talking about .NET 9 work, but I'd like to get these updates in ASAP given that the live doc is currently broken 💥.

Alternatively, I can remove that scenario (section) from the doc (i.e., comment it out), and we just won't support the approach for this release.

@guardrex
Copy link
Collaborator Author

guardrex commented Jan 19, 2024

@MackinnonBuck ... I'm going to go ahead with this because the article is broken 💥 until this goes live. I don't want to bug out for the weekend leaving the article in this state. I'll issue a patch PR next week if it's determined that a no-op App component isn't the best way to address the "Bug 2" scenario.

@guardrex guardrex merged commit a978a69 into main Jan 19, 2024
3 checks passed
@guardrex guardrex deleted the guardrex/blazor-integration branch January 19, 2024 21:25
@MackinnonBuck
Copy link
Member

It sounds like you're talking about .NET 9 work, but I'd like to get these updates in ASAP given that the live doc is currently broken 💥

Yeah, the empty App component approach seems fine. We could alternatively document the _Imports suggestion I had, but I'll leave it up to you to decide which approach you feel is more understandable 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Must use the @namespace for components in MVC app
2 participants