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

Improve Blazor project template #52234

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Improve Blazor project template #52234

merged 5 commits into from
Nov 28, 2023

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Nov 21, 2023

Use consistent code style in Blazor project templates

  • Removed usage of top-level statements in client project if "Do not use top-level statements" is selected
  • Removed extra "@" in @render-mode values
  • Always use typeof(Namespace._Imports).Assembly instead of typeof(Counter).Assembly so the compilation does not break when the sample Counter component is removed, and so the code is more consistent with how it is when no sample content is generated
  • Added Account/AccessDenied endpoint to individual auth option to match Identity UI razor pages. This is shown when the user is authenticated but unauthorized by default.

Fixes #52079
Fixes #52084
Fixes #52167

Customer Impact

In addition to not using top-level statements when the customer requests that we don't, this improves code style consistency within the Blazor project template and with the Blazor docs.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

These are small stylistic changes to the Blazor project templates.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 21, 2023
@ghost ghost added this to the 8.0.x milestone Nov 21, 2023
@ghost
Copy link

ghost commented Nov 21, 2023

Hi @halter73. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@halter73
Copy link
Member Author

@guardrex Regarding the following from #51084

Second ❓: WRT the logic for finding the .Client project assembly ...

#if (UseWebAssembly && SampleContent)
    .AddAdditionalAssemblies(typeof(Counter).Assembly);
#elif (UseWebAssembly)
   .AddAdditionalAssemblies(typeof(BlazorWeb_CSharp.Client._Imports).Assembly);
#endif

I don't see why the latter _Imports file-based approach isn't adopted even if sample content is present. After all, even the empty solution (no sample pages) has an _Imports file. As soon as the dev 🔪 out the Counter component because any production app isn't going to keep it, it will 💥. Many new devs and new-to-Blazor devs won't know the second approach for the _Imports file off the top of their head.

I don't have a huge preference one way or the other. I think @SteveSandersonMS chose Counter initially, and then I later added _Imports for the non-sample case. I left it as Counter for the default case because it does look better, but if everyone is okay with changing it to always use _Imports, I can do that as part of this PR.


namespace BlazorWeb_CSharp.Client;

class Program
Copy link
Member Author

Choose a reason for hiding this comment

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

Unlike the server template, this does not have an accessibility modifier. Neither does the Main method. I would have made both the class and public like we do for the server project, but this made the @typeof(Program).Assembly reference in Routes.razor ambiguous.

<Router AppAssembly="@typeof(Program).Assembly" AdditionalAssemblies="new[] { typeof(Client._Imports).Assembly }">

I could change that to be @typeof(BlazorWeb_CSharp.Program).Assembly instead and make this public. And then only qualify it when you've disabled top level statements.

Does anyone prefer that, or is everyone fine the way it is?

@danroth27
Copy link
Member

@halter73 Can you update Routes.razor as well to remove the unnecessary leading @ chars?:

<Router AppAssembly="typeof(Program).Assembly">
    <Found Context="routeData">
        <RouteView RouteData="routeData" DefaultLayout="typeof(Layout.MainLayout)" />
        <FocusOnNavigate RouteData="routeData" Selector="h1" />
    </Found>
</Router>

@guardrex
Copy link
Contributor

@danroth27, we discussed the delta that removing those @ will create with the docs. If you go without them in the project template, shouldn't I make the doc set match? I'd kind'a prefer to do it because readers will spot it and open issues (and even patch PRs) for what they think are doc bugs.

@guardrex
Copy link
Contributor

guardrex commented Nov 21, 2023

@halter73 ... In addition to any template change, I've opened a doc issue to improve additional assemblies coverage at ...

BTW, I don't think that section title is accurate ...

Discover components from additional assemblies for Static Server rendering

Shouldn't that just state ...

Discover components from additional assemblies

@danroth27
Copy link
Member

@danroth27, we discussed the delta that removing those @ will create with the docs. If you go without them in the project template, shouldn't I make the doc set match? I'd kind'a prefer to do it because readers will spot it and open issues (and even patch PRs) for what they think are doc bugs.

@guardrex Yeah, we're trying to standardize on a consistent style here. The style we're going with is to only include the @ if it's needed.

@halter73 halter73 added the Servicing-consider Shiproom approval is required for the issue label Nov 22, 2023
@ghost
Copy link

ghost commented Nov 22, 2023

Hi @halter73. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@halter73 halter73 added area-blazor Includes: Blazor, Razor Components and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Nov 25, 2023
@guardrex
Copy link
Contributor

guardrex commented Nov 27, 2023

@halter73 @javiercn ... Passing note and ❓ before we go here.

These definitions are similar WRT locating components ...

AdditionalAssemblies param on the Blazor router instance ...

Gets or sets a collection of additional assemblies that should be searched for components that can match URIs.

AddAdditionalAssemblies method for the endpoint convention builder ...

Summary ...

Adds the given additional assemblies to the component application.

Remark ...

The provided assemblies will be scanned for pages that will be mapped as endpoints.

Two questions ...

  1. The API remark for the AddAdditionalAssemblies method makes it sound like one wouldn't need to specify the AdditionalAssemblies param on the router in order to discover routable client-side components. I tried removing the param from the router, leaving just the call to AddAdditionalAssemblies, and it worked. What should we say about this? Should we just say in a BWA that an assembly should be indicated in both spots anyway? ... which is what the project template sets up by default.

  2. In the same vein as question 2, should component libraries also have their assemblies indicated to the AdditionalAssemblies parameter if the dev indicates the library's assembly to the AddAdditionalAssemblies method?

UPDATE: I placed a PR with draft improved language at dotnet/AspNetCore.Docs#31131.

@danroth27
Copy link
Member

danroth27 commented Nov 27, 2023

@guardrex I believe you need to specify AdditionalAssemblies on the Router to discover components in other assemblies when the Router is set up to be interactive, either from the server or the client. If the Router doesn't have an interactive render mode, then AddAdditionalAssemblies on MapRazorComponents should be sufficient.

@guardrex
Copy link
Contributor

@danroth27 ... Thanks. I understand. I'll make further updates shortly and ping for review on the PR.

@guardrex
Copy link
Contributor

guardrex commented Nov 27, 2023

Mmmmmmmm 🤔 ... Idk ... that doesn't sound like it matches the behavior.

HOLD for a sec. I need to research further. I lost a test app. I'll post back in a bit.

@guardrex
Copy link
Contributor

Ok ... I think I get it now. I'll make the updates the PR and ping for review.

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Nov 28, 2023
@ghost
Copy link

ghost commented Nov 28, 2023

Hi @halter73. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@mkArtakMSFT mkArtakMSFT merged commit c909955 into release/8.0 Nov 28, 2023
26 checks passed
@mkArtakMSFT mkArtakMSFT deleted the halter73/52079 branch November 28, 2023 03:32
@ghost ghost modified the milestones: 8.0.x, 8.0.1 Nov 28, 2023
halter73 added a commit that referenced this pull request Feb 14, 2024
# Use consistent code style in Blazor project templates

- Removed usage of top-level statements in client project if "Do not use top-level statements" is selected
- Removed extra "@" in `@render-mode` values
- Always use `typeof(Namespace._Imports).Assembly` instead of `typeof(Counter).Assembly` so the compilation does not break when the sample Counter component is removed, and so the code is more consistent with how it is when no sample content is generated
- Added Account/AccessDenied endpoint to individual auth option to match Identity UI razor pages. This is shown when the user is authenticated but unauthorized by default.

Fixes #52079
Fixes #52084
Fixes #52167

## Customer Impact

In addition to not using top-level statements when the customer requests that we don't, this improves code style consistency within the Blazor project template and with the Blazor docs.

## Regression?

- [ ] Yes
- [x] No

## Risk

- [ ] High
- [ ] Medium
- [x] Low

These are small stylistic changes to the Blazor project templates.

## Verification

- [x] Manual (required)
- [ ] Automated

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [x] N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components Servicing-approved Shiproom has approved the issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants