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

Update "empty" web project templates to use minimal hostinng and routing #32003

Merged
merged 10 commits into from
Apr 23, 2021

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Apr 21, 2021

Description

Update the "Empty" project templates (used by dotnet new empty/dotnet new web and VS) to use the new "minimal hosting" APIs that were just merged minutes ago for preview4 by #31825.

Customer Impact

We're looking to collect feedback on our new "minimal hosting" and "minimal routing" API which we will present at BUILD. This updates the "empty" web project templates to make it easier to customer trying preview4 after BUILD to test out these new APIs.

Regression?

  • Yes
  • No

The template behaves as it did before. It does not include all the usings/opens for unused namespace. That might change based on code review feedback.

Risk

  • High
  • Medium
  • Low

This is a pretty big change to the empty web project template. It might confuse people. It is just a template change though.

@glennc looked up some usage statistics and this is one of the less popular than the MVC and API templates.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Addresses #30665

@ghost
Copy link

ghost commented Apr 21, 2021

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@halter73 halter73 removed the Servicing-consider Shiproom approval is required for the issue label Apr 21, 2021
if app.Environment.IsDevelopment() then
app.UseDeveloperExceptionPage() |> ignore

app.MapGet("/", Func<string>(fun () -> "Hello World!")) |> ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the explicit Func construction needed here? Might be able to elide that

Copy link
Member Author

@halter73 halter73 Apr 21, 2021

Choose a reason for hiding this comment

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

It seems like it. Without it, I see the following error:

Program.fs(12,34): error FS0001: This expression was expected to have type↔    'AspNetCore.Http.HttpContext'    ↔but here has type↔    'unit' [C:\dev\dotnet\aspnetcore\src\Http\samples\MinimalSampleFSharp\MinimalSampleFSharp.fsproj]

Copy link
Member

@davidfowl davidfowl Apr 21, 2021

Choose a reason for hiding this comment

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

This is the F# language feature we wanted

src/Http/samples/MinimalSampleFSharp/Program.fs Outdated Show resolved Hide resolved
}

app.MapGet("/", (Func<string>)(() => "Hello World!"));
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like that we still have to explicitly cast to Func<string> until we can consume the changes in dotnet/roslyn#52448, but I still think this is already a big improvement overall.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The bits should be lining up for preview 4 to allow this cast to be removed.


app.MapGet("/", (Func<string>)(() => "Hello World!"));

await app.RunAsync();
Copy link
Member

Choose a reason for hiding this comment

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

Program+Startup before: 66 lines
after: 14 lines.

It's a lot emptier than it used to be 😁.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Make the "empty" project, "really empty"

@ShreyasJejurkar
Copy link
Contributor

Go..Go..Go SingleFile 😅😅

@jkotalik
Copy link
Contributor

This plus making Kestrel the default server brings a smile to my face 😄. True modernization.

@halter73
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -1,9 +1,9 @@
{
"sdk": {
"version": "6.0.100-preview.4.21216.8"
"version": "6.0.100-preview.4.21222.6"
Copy link
Member Author

Choose a reason for hiding this comment

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

Heads up @dotnet/aspnet-build. I'm updating the preview4 branch SDK version so we can consume the WebApplication related types from our new "empty" web project templtates.

Copy link
Member

Choose a reason for hiding this comment

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

If these new types come from dotnet/aspnetcore, something is very wrong if they aren't just showing up. Fine if, on the other hand, the main thing we need is @DavidKarlas' dotnet/templating#3078 fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to update this to get EmptyWebTemplateTest.EmptyWebTemplateCSharp EmptyWebTemplateTest.EmptyWebTemplateFSharp passing locally and on helix, so there's definitely room for improvement here. Not sure how common it is to update the templates to use very-newly-added APIs.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding of the fix to templating is that the regression broke the custom hive logic we were using to make our template tests find our updated templates, so that's why stefan had to update the sdk. The templating workaround PR should also fix the tests I would assume

@Pilchie
Copy link
Member

Pilchie commented Apr 22, 2021

👀

Latest `dotnet new` doesn't work with relative path for `--debug:custom-hive`... Hence make it absolute
Also switched to using `--debug:disable-sdk-templates` to avoid potential conflict between SDK and tests installed templates.
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Looks fine from a build perspective

@@ -1,9 +1,9 @@
{
"sdk": {
"version": "6.0.100-preview.4.21216.8"
"version": "6.0.100-preview.4.21222.6"
Copy link
Member

Choose a reason for hiding this comment

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

If these new types come from dotnet/aspnetcore, something is very wrong if they aren't just showing up. Fine if, on the other hand, the main thing we need is @DavidKarlas' dotnet/templating#3078 fix.

@halter73 halter73 added the Servicing-consider Shiproom approval is required for the issue label Apr 23, 2021
@ghost
Copy link

ghost commented Apr 23, 2021

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed Servicing-consider Shiproom approval is required for the issue labels Apr 23, 2021
@Pilchie Pilchie merged commit 90137b0 into release/6.0-preview4 Apr 23, 2021
@Pilchie Pilchie deleted the halter73/30665 branch April 23, 2021 04:55
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-hosting feature-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.