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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dotnet-8 KeyedSevice #38333

Merged
merged 4 commits into from Nov 30, 2023
Merged

Update dotnet-8 KeyedSevice #38333

merged 4 commits into from Nov 30, 2023

Conversation

WeihanLi
Copy link
Contributor

@WeihanLi WeihanLi commented Nov 20, 2023

Summary

  • Update to IServiceProvider since there's no IKeyedServiceProvider registered
  • Update the SmallCacheConsumer to get cache value instead of returning the IMemoryCache

Internal previews

馃搫 File 馃敆 Preview link
docs/core/whats-new/dotnet-8.md What's new in .NET 8

there's no IKeyedServiceProvider registered
@dotnet-bot dotnet-bot added this to the November 2023 milestone Nov 20, 2023
@ghost ghost added the community-contribution Indicates PR is created by someone from the .NET community. label Nov 20, 2023
update SmallCacheConsumer
@gewarren
Copy link
Contributor

@ericstj Can you review this?

@ericstj
Copy link
Member

ericstj commented Nov 27, 2023

Looks like this sample still doesn't work. Seems the BigCache and SmallCache types are still missing? perhaps those were meant to be https://github.com/dotnet/AspNetCore.Docs.Samples/blob/d23c059e7e88a4151ab4adbf5a56d27dc4d726c9/samples/WebKeyedService/ICache.cs#L1C1-L13C2 as ICache?

Here's an update that works for me https://gist.github.com/ericstj/efd848d65ba6591546a2707897cad8fa. I'm not entirely sure if this communicates the concepts as cleanly. @steveharter @benjaminpetit what do you think?

@WeihanLi
Copy link
Contributor Author

Oh yeah, thanks for the careful review @ericstj
it is, I only focused on the KeyedService usage, not trying to run up the sample code, my bad
updated sample code according to your gist code and verified it works, thanks very much

@ericstj
Copy link
Member

ericstj commented Nov 28, 2023

Thank you for the fix here. That original sample was something we copied from a tweet - not surprised it had multiple issues 馃槅. I would like either @steveharter or @benjaminpetit to weigh in on this before we merge.

@benjaminpetit
Copy link
Member

I think we should also show that you can use the FromKeyedServices attribute directly in the MapGet method:

app.MapGet("/big", ([FromKeyedServices("big")] ICache cache) => cache.Get("data"));

But also showing the attribute usage in the constructor is important. So maybe we could so one of each, if that's not too weird?

@WeihanLi
Copy link
Contributor Author

Make sense, how about adding the following two examples?

app.MapGet("/big-cache", ([FromKeyedServices("big")] ICache cache) => cache.Get("data"));
app.MapGet("/small-cache", (HttpContext httpContext) => httpContext.RequestServices.GetRequiredKeyedService<ICache>("small").Get("data"));

@benjaminpetit
Copy link
Member

Yes that would be great!

update keyedService example
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM - also tested this in a web project and all is working well

@gewarren gewarren merged commit 1694b73 into dotnet:main Nov 30, 2023
8 checks passed
@WeihanLi WeihanLi deleted the patch-2 branch December 1, 2023 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates PR is created by someone from the .NET community. dotnet-fundamentals/svc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants