Skip to content

Update to .NET 8 and leverage keyed services for decoration#209

Merged
khellang merged 22 commits into
masterfrom
feature/net8.0
Sep 25, 2024
Merged

Update to .NET 8 and leverage keyed services for decoration#209
khellang merged 22 commits into
masterfrom
feature/net8.0

Conversation

@khellang

@khellang khellang commented Aug 18, 2023

Copy link
Copy Markdown
Owner

What do you think @DanHarltey?

Closes #171
Closes #208
Closes #65
Closes #200

@khellang khellang changed the title Whitespace and line break tweaks Update to .NET 8 and leverage keyed services for decoration Aug 18, 2023
@codecov

codecov Bot commented Aug 18, 2023

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 70.49180% with 36 lines in your changes missing coverage. Please review.

Project coverage is 66.15%. Comparing base (a634e58) to head (7fcedf7).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/Scrutor/TypeSourceSelector.cs 17.64% 14 Missing ⚠️
src/Scrutor/ServiceDescriptorExtensions.cs 35.71% 7 Missing and 2 partials ⚠️
.../Scrutor/ServiceCollectionExtensions.Decoration.cs 69.56% 5 Missing and 2 partials ⚠️
src/Scrutor/LifetimeSelector.cs 0.00% 3 Missing ⚠️
src/Scrutor/ClosedTypeDecorationStrategy.cs 83.33% 1 Missing ⚠️
src/Scrutor/DecorationStrategy.cs 94.11% 0 Missing and 1 partial ⚠️
src/Scrutor/OpenGenericDecorationStrategy.cs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   64.71%   66.15%   +1.44%     
==========================================
  Files          25       24       -1     
  Lines        1278     1232      -46     
  Branches        0      112     +112     
==========================================
- Hits          827      815      -12     
+ Misses        451      387      -64     
- Partials        0       30      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DanHarltey DanHarltey left a comment

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.

Nice. A lot nicer than the DecoratedType file, it is nasty. Surprised by how little of the code actually changes. This should allow more compatibility.

Comment thread .github/workflows/build.yml Outdated
Comment thread src/Scrutor/ServiceCollectionExtensions.Decoration.cs Outdated
Comment thread src/Scrutor/ServiceDescriptorExtensions.cs Outdated
@khellang

khellang commented Sep 1, 2023

Copy link
Copy Markdown
Owner Author

Thanks for the review, @DanHarltey! All good feedback 👍🏻

@dario-l

dario-l commented Oct 11, 2023

Copy link
Copy Markdown

Hi @khellang
When this will be available at the nuget package? We really need this. 😃

Please, please, please :)

PS
We are doing future project but as for now we are using NET8 RC2. Before our release we are switching to full NET8 ofcoz. 😄

@khellang

Copy link
Copy Markdown
Owner Author

I can probably target RC2 and publish a prerelease version if anyone's eager to test it out.

@dario-l

dario-l commented Oct 11, 2023

Copy link
Copy Markdown

I can probably target RC2 and publish a prerelease version if anyone's eager to test it out.

@khellang Yes please 🙏🏻

@dario-l

dario-l commented Oct 12, 2023

Copy link
Copy Markdown

Hi @khellang 😃
I'm just cloned branch for net8 and adopted source code into my project. 😄

Now I can wait for released version of net8 and newest scrutor 😃
I hope that Scan will have appropriate extensions for keyed services also. 👍🏻
Thanks for help. ❤️

@khellang

Copy link
Copy Markdown
Owner Author

I hope that Scan will have appropriate extensions for keyed services also. 👍🏻

What does that mean? How do you envision using keyed services with Scrutor?

@dario-l

dario-l commented Oct 12, 2023

Copy link
Copy Markdown

I hope that Scan will have appropriate extensions for keyed services also. 👍🏻

What does that mean? How do you envision using keyed services with Scrutor?

I would love to register services from given assembly as keyed with specified key,

For example. I have modular monolith. Each module is a separated project/assembly. I execute services.Scan(...) to register all ICommandHandler from that assembly. I want to register them as keyed services. Thanks to that I can resolve only those services in particular module.

For now I am using this hacky solution 😄

        var tempServices = new ServiceCollection();

        tempServices.Scan(  );
        
        foreach (var service in tempServices)
        {
                // register service as keyed
        }

It would be great to do this something like:

new ServiceCollection()
            .Scan(s => s.FromAssembliesOf(module.GetType())
                .AddClasses(c =>
                {
                    c.AssignableToAny(
                            typeof(ICommandHandler<,>),
                            typeof(ICommandHandler<>),
                            typeof(IQueryHandler<,>),
                            typeof(IEventHandler<>),
                            typeof(IDomainEventHandler<>))
                        .WithoutAttribute<DecoratorAttribute>();
                }, false)
                .AsKeyed(moduleName) //  <========
                .AsImplementedInterfaces()
                .WithScopedLifetime());

@khellang

Copy link
Copy Markdown
Owner Author

If it's allowed to register multiple services against the same key, it could work. I'll have to verify it.

@dario-l

dario-l commented Oct 19, 2023

Copy link
Copy Markdown

Hi @khellang :)
How about this? Will be any chance for this? 😃

@dario-l

dario-l commented Nov 15, 2023

Copy link
Copy Markdown

Hi @khellang ?

How about now, after NET8 premiere? 😄

@rekosko

rekosko commented Jan 12, 2024

Copy link
Copy Markdown

Still waiting :)

@khellang

Copy link
Copy Markdown
Owner Author

@rekosko What exactly are you waiting for? The latest version on NuGet works fine on .NET 8, right?

@rekosko

rekosko commented Jan 12, 2024

Copy link
Copy Markdown

@khellang yes I confirm it works on .NET 8 :) But decoration for keyed services is not yet implemented or I missed the purpose of this PR?

@khellang

Copy link
Copy Markdown
Owner Author

That's not the purpose of this PR, no. Although I want to add that as well before shipping a new version. This PR replaces an old hack to avoid StackOverflowExceptions when decorating services with keyed services under the covers. It also files a few other long-standing issues and pending breaking changes.

@khellang

Copy link
Copy Markdown
Owner Author

BTW, if anyone wants to contribute support for decorating keyed services to get a release out sooner, that would be cool.

@savornicesei

savornicesei commented Feb 8, 2024

Copy link
Copy Markdown
Contributor

@khellang Can this be merged into master or it's not complete or it needs more testing? Or it's better to base all the tests and work on feature/net8.0 branch?

@khellang

khellang commented Feb 8, 2024

Copy link
Copy Markdown
Owner Author

It's not totally complete and needs more tests. It's better to base work off this branch for now. Once this goes into master, I will release a new major version.

@abatishchev

Copy link
Copy Markdown

Gentle ping on this. I wonder what's the plan/fate for this PR?

@spko

spko commented Sep 20, 2024

Copy link
Copy Markdown

Howdy! Will Scrutor be ported to .NET 8?
.NET 6 is getting very close to its End of Support deadline.

@khellang

khellang commented Sep 20, 2024

Copy link
Copy Markdown
Owner Author

Will Scrutor be ported to .NET 8? .NET 6 is getting very close to its End of Support deadline.

Just because the library targets .NET 6, doesn't mean it doesn't support .NET 8. It's perfectly fine to use packages targeting older framework versions on newer frameworks.

Have you encountered any issues using the package on .NET 8?

@khellang khellang enabled auto-merge (rebase) September 25, 2024 07:11
@khellang khellang merged commit 32f5b8b into master Sep 25, 2024
@khellang khellang deleted the feature/net8.0 branch September 25, 2024 07:22
@khellang

khellang commented Sep 25, 2024

Copy link
Copy Markdown
Owner Author

Everyone, I've gone ahead and merged this and released v5.0.0 to NuGet. Please try it out and report back if you find any issues 🙏🏻 Thanks for your patience 😊

@ThumbGen

Copy link
Copy Markdown

Is there an example somewhere how to perform 'keyed registrations' using the latest Scrutor version? TIA

@khellang

Copy link
Copy Markdown
Owner Author

As mentioned in #209 (comment), this isn't adding support for keyed registrations, it's merely (ab)using them internally to avoid StackOverflowExceptions when doing decoration 😄

If someone is up to the task to implement support for keyed registrations, I'd love to see it ❤️

Do you have any thoughts on what it would look like, @ThumbGen?

@ThumbGen

Copy link
Copy Markdown

Thanks for the fast reply. Sadly I have no thoughts on it, I am just an avid consumer of this lovely library :-(

@julealgon

Copy link
Copy Markdown

@khellang I saw this PR mentioned in the 5.0.1 release notes and just wanted to make sure I get this right: this change is just to rely on keyed service capabilities internally, but it results in the exact same behavior as before, including all the limitations that existed before. Is that a valid way of putting it?

If not, could you elaborate on what this could end up changing or what limitations it removes in terms of using decorators?

@khellang

khellang commented Nov 7, 2024

Copy link
Copy Markdown
Owner Author

Is that a valid way of putting it?

@julealgon That's correct, yes. Earlier, we relied internally on a huge hack to make decoration work, which ended up breaking a few scenarios, like Azure Functions. This PR replace that solution with a less hacky solution based on keyed services.

There is a potential benefit of this, which is that you could resolved a keyed service to get a hold of the "original", wrapped, service, but that hasn't been tested fully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

10 participants