Adds TodosApi app (native AOT "goldilocks" stage 2) #1828
Conversation
DamianEdwards
requested review from
davidfowl,
JamesNK,
mitchdenny,
sebastienros and
eerhardt
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.0-preview.3.23177.8" /> |
There was a problem hiding this comment.
is there a way to keep this version up to date with daily builds?
There was a problem hiding this comment.
We can use a wildcard, e.g. 8.0.0-preview.*?
There was a problem hiding this comment.
Seems there's a $(MicrosoftAspNetCoreAppPackageVersion) property defined, I'll try using that.
There was a problem hiding this comment.
Oh it's hard-coded to a preview.1 version 😱
There was a problem hiding this comment.
I think that might get replaced by crank.... @sebastienros ?
There was a problem hiding this comment.
Yeah I've noticed the projects aren't always set up for ease of local use so I made some tweaks to improve that as part of this PR now, i.e. ensuring there are appropriate package versions set.
There was a problem hiding this comment.
You can have a default version set for local development, and fallback to $(MicrosoftAspNetCoreAppPackageVersion) if it's defined. Crank will set it on builds. There is the same one for the runtime.
There was a problem hiding this comment.
I humbly suggest we shouldn't be relying on anything local being manually configured to get a good dev experience. The solution/projects should be openable locally after cloning without getting warnings.
There was a problem hiding this comment.
@sebastienros are you suggesting we should change all the package version references in the app project files to hard-code a default version (can include a wildcard) but prefer $(MicrosoftAspNetCoreAppPackageVersion) if it's set? It's a shame we can't easily make that more elegant in MSBuild but it will certainly achieve what I think we're looking for.
There was a problem hiding this comment.
I think you can have something like
<MicrosoftAspNetCoreAuthenticationJwtBearerVersion>8.0.0-preview.3.23177.8</MicrosoftAspNetCoreAuthenticationJwtBearerVersion>
<MicrosoftAspNetCoreAuthenticationJwtBearerVersion Condition="'$(MicrosoftAspNetCoreAppPackageVersion)' != ''">$(MicrosoftAspNetCoreAppPackageVersion)</MicrosoftAspNetCoreAuthenticationJwtBearerVersion>
This way it will build locally, but use the version aligned with aspnet when using crank.
| <PropertyGroup Condition="$(TargetFramework) == 'net8.0'"> | ||
| <MicrosoftAspNetCoreAppPackageVersion>8.0.0-preview.1.23112.2</MicrosoftAspNetCoreAppPackageVersion> | ||
| <PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'"> | ||
| <MicrosoftAspNetCoreAppPackageVersion>$(MicrosoftAspNetCoreAppPackageVersion80)</MicrosoftAspNetCoreAppPackageVersion> |
There was a problem hiding this comment.
Works, but why treat net8 differently with a separate constant?
There was a problem hiding this comment.
I added a net8 variant so it could be referenced elsewhere when a TargetFramework is not passed in from the outside (MSBuild order of evaluation, etc.).
| var group = routes.MapGroup("/api/todos"); | ||
|
|
||
| group.MapGet("/", (NpgsqlDataSource db, CancellationToken ct) => db.QueryAsync<Todo>("SELECT * FROM Todos", ct)) | ||
| .WithName("GetAllTodos"); |
There was a problem hiding this comment.
What does the WithName do? Do we need it?
There was a problem hiding this comment.
It sets the route name and endpoint name metadata which can be used for route resolution, URL generation, Open API metadata, etc. Not strictly required in this app at the moment as we aren't doing any of those things.
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
|
LGTM. Interested in getting this merged and seeing RPS with workstation GC on APIs that do some work (auth and/or data access) |
Co-authored-by: Nino Floris <mail@ninofloris.com>
|
Numbers from a manual run on
|
|
Judging by requests being equal to bad responses, it doesn't seem to work. |
For the non-vanilla runs you're right 😞 Hopefully something straightforward. |
|
@vonzshik ah that's expected actually! Right now JSON serialization doesn't work with |
|
The JSON support for |
If that was indeed the case, wouldn't non-trimmed also fail? + judging by the first published results (on powerbi), crank does use the runtime with the fix, but there are still errors. |
|
@vonzshik there are two issues. The first is that when trimming or native AOT is enabled, the JSON source generator doesn't support |
|
Seems there's an issue with the runtime support for JSON serializing |
|
I suppose we change to IEnnumerable for now and hopefully everything works? |
|
@vonzshik I'm inclined to leave it as is for now and focus on getting the underlying issue fixed (seems it's actually ASP.NET Core that needs a code change to fix this). |
|
Well then, let's pray that there are no other issues and fixing that isn't going to take long... |
This adds a refactored version of the TrimmedTodo app configured to align with our goals for the native AOT "stage 2" app.
The app uses:
When published native AOT with the
-p:EnableLogging=trueproperty, the exe size is currently 20,135 KB on Windows. Everything except the JWT authentication and data access works in native AOT right now, the latter will start working once the JSON support forIAsyncEnumerablelands.Native AOT published sizes from Crank runs: