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 giraffe sample to .NET 6 alpha #6891

Merged
merged 3 commits into from
Nov 14, 2021
Merged

Conversation

forki
Copy link
Contributor

@forki forki commented Nov 9, 2021

This updates the Giraffe sample to .NET 6 as soon as Giraffe 6 comes out

@forki forki force-pushed the fsharp6 branch 6 times, most recently from a31d507 to c726a94 Compare November 10, 2021 09:26
@forki forki changed the title WIP: Update giraffe sample to .NET 6 Update giraffe sample to .NET 6 alpha Nov 10, 2021
@forki
Copy link
Contributor Author

forki commented Nov 13, 2021

this one is ready

@NateBrady23 NateBrady23 merged commit be20753 into TechEmpower:master Nov 14, 2021
@forki forki deleted the fsharp6 branch November 15, 2021 14:44
|> ignore)
.Build()
.Run()
let configureApp (appBuilder : IApplicationBuilder) =
Copy link

Choose a reason for hiding this comment

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

Glancing at this, is there anything here that could cause a performance change? UseKestrel being dropped? Anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK that's how hosting is initialized in asp.net core 6. /cc @davidfowl

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a bit more minimal overall:

let builder = WebApplication.CreateBuilder(args)

builder.Services.AddGiraffe()
                .AddSingleton(jsonSerializer) |> ignore

let app = builder.Build()

app.UseGiraffe HttpHandlers.endpoints |> ignore

app.Run()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another PR coming that disables logging. Could this be a change to asp.net core 5 that affected performance? Was logging enabled before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And thanks for the help!

Copy link
Contributor

@slang25 slang25 Nov 17, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidfowl AddRouting could indeed go but removing UseRouting errors with:

Unhandled exception. System.InvalidOperationException: EndpointRoutingMiddleware matches endpoints setup by EndpointMiddleware and so must be added to the request execution pipeline before EndpointMiddleware. Please add EndpointRoutingMiddleware by calling 'IApplicationBuilder.UseRouting' inside the call to 'Configure(...)' in the application startup code.

Are we expected to add this inside the UseGiraffe implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to MapGiraffeEndpoints instead of UseGiraffe

cc @dustinmoris

Copy link
Contributor

Choose a reason for hiding this comment

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

When the EndpointRouting namespace is open then the UseGiraffe extension method is doing the correct thing already.

jbr pushed a commit to trillium-rs/FrameworkBenchmarks that referenced this pull request Mar 7, 2022
* Update giraffe sample to .NET 6

* Use WebApplication in Giraffe host

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

Successfully merging this pull request may close these issues.

7 participants