close
Skip to content

Blazor united initial startup apis for registering pages#46996

Merged
javiercn merged 10 commits into
mainfrom
blazor-united-feature-branch
Mar 7, 2023
Merged

Blazor united initial startup apis for registering pages#46996
javiercn merged 10 commits into
mainfrom
blazor-united-feature-branch

Conversation

@javiercn
Copy link
Copy Markdown
Member

@javiercn javiercn commented Mar 2, 2023

Brings in the initial implementation for Components.Endpoints.

Note that the goal of this PR is to focus on providing the initial structure and not a proper implementation of any of the concepts. There are links in comments pointing to issues that provide more context and details.

I have stopped at the point where we have a sample that is able to discover components in the app and render a static page. We will be filling in the gaps in comments that are captured in other issues.

@javiercn javiercn requested review from a team, dougbu and wtgodbe as code owners March 2, 2023 14:50
@javiercn javiercn marked this pull request as draft March 2, 2023 14:52
@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Mar 2, 2023
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
<Description>Server side rendering for ASP.NET Core Components.</Description>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why include a $(Description) if this isn't going into its own package❔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is following the template from other projects in this folder. It's fine to leave it even if we do not need it.

Comment thread src/Components/Endpoints/src/Microsoft.AspNetCore.Components.Endpoints.csproj Outdated
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm sure this project will be successful across the board 😁:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Are you implying there is something wrong?

I copied the code from another test project

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think he's just remarking that it's a completely empty test project so it's always going to pass :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think he's just remarking that it's a completely empty test project so it's always going to pass :)

exactly

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It has a couple of tests now 😄.

This PR is only adding all the scaffold for the things we need.

@javiercn javiercn force-pushed the blazor-united-feature-branch branch from 4a490b3 to 9239fb9 Compare March 6, 2023 12:18
@javiercn javiercn marked this pull request as ready for review March 6, 2023 13:58
@javiercn javiercn requested a review from a team as a code owner March 6, 2023 13:58
@javiercn
Copy link
Copy Markdown
Member Author

javiercn commented Mar 6, 2023

@wtgodbe do you have any idea why the shared framework tests fail?

I tried to poke around, but I am not too familiar to how the SharedFx builds and can't find a way to solve it.

"Microsoft.AspNetCore.Authorization.Policy",
"Microsoft.AspNetCore.Components",
"Microsoft.AspNetCore.Components.Authorization",
"Microsoft.AspNetCore.Components.Endpoints",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@javiercn Add this to ListedTargetingPackAssemblies below as well (around line 177)

Copy link
Copy Markdown
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

@javiercn So far this all looks perfect to me.

Do you plan to add an E2E test showing that your new endpoint gets hit? It doesn't necessarily have to use a browser - it could just do an HttpClient request to an E2E test app, like we do for the other prerendering tests. But no worries if you prefer that to be handled in a separate PR.

@javiercn javiercn merged commit f772f1b into main Mar 7, 2023
@javiercn javiercn deleted the blazor-united-feature-branch branch March 7, 2023 09:58
@ghost ghost added this to the 8.0-preview3 milestone Mar 7, 2023
@javiercn
Copy link
Copy Markdown
Member Author

javiercn commented Mar 7, 2023

@SteveSandersonMS let's handle that one separately, as we start getting into more details. Need to check the approach we use in MVC.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants