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

[API Proposal]: Reconciling base classes and interfaces in Microsoft.Extensions #57596

Open
terrajobst opened this issue Aug 17, 2021 · 9 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Primitives
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Aug 17, 2021

Background and motivation

We keep getting asked to add functionality to features in Microsoft.Extensions, such as:

We can't add methods to interfaces, so we're adding more and more APIs to the classes only. We should make a decision on what to do with the interfaces. My proposal is hide and obsolete.

API Proposal

TBD

API Usage

TBD

Risks

No response

@terrajobst terrajobst added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 17, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 17, 2021
@terrajobst terrajobst added this to the 7.0.0 milestone Aug 17, 2021
@jeffschwMSFT jeffschwMSFT added area-Extensions-Primitives and removed untriaged New issue has not been triaged by the area owner labels Aug 17, 2021
@ghost
Copy link

ghost commented Aug 17, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

We keep getting asked to add functionality to features in Microsoft.Extensions, such as:

We can't add methods to interfaces, so we're adding more and more APIs to the classes only. We should make a decision on what to do with the interfaces. My proposal is hide and obsolete.

API Proposal

TBD

API Usage

TBD

Risks

No response

Author: terrajobst
Assignees: -
Labels:

api-suggestion, area-Extensions-Primitives

Milestone: 7.0.0

@eerhardt
Copy link
Member

cc @davidfowl @Tratcher

@Tratcher
Copy link
Member

IMemoryCache is used in many public TagHelper APIs like:
https://github.com/dotnet/aspnetcore/blob/8b30d862de6c9146f466061d51aa3f1414ee2337/src/Mvc/Mvc.TagHelpers/src/DistributedCacheTagHelper.cs#L44-L46

Most other ASP.NET uses seem to be internal.

@eerhardt
Copy link
Member

My proposal is hide and obsolete.

I can see 2 high-level approaches to fixing this.

1. Embrace the interfaces and use DIMs

Taking #45593 as a case study, it would mean this change:

namespace Microsoft.Extensions.Caching.Memory
{
    public interface IMemoryCache
    {
+#if NET6_OR_GREATER
+        void Clear() { throw new NotImplementedException(); }
+#endif
    }

    public class MemoryCache : IMemoryCache
    {
+        public void Clear();
    }
}

The advantages:

  • Consumers don't need to rewrite their code away from the interfaces.
  • We would be dogfooding the DIM feature, which was implemented exactly for this scenario

Disadvantages:

  • People consuming the abstraction (IMemoryCache) on older TFMs (i.e. netstandard2.0) wouldn't be able to use the new members
  • Risk that we hit issues with DIMs, as we don't have much experience since we haven't used them

2. Deprecate the interfaces and replace with base classes

Again, using #45593 as a case study:

namespace Microsoft.Extensions.Caching.Memory
{
+   [Obsolete("Use MemoryCacheBase instead")]
    public interface IMemoryCache
    {
...
    }

+    public abstract class MemoryCacheBase
+    {
+        public virtual void Clear() { throw new NotImplementedException(); }
+    }

-   public class MemoryCache : IMemoryCache
+   public class MemoryCache : MemoryCacheBase, IMemoryCache
    {
    }

(Note: we wouldn't strictly need to obsolete the interfaces. So we have options 2.a - obsolete interfaces and 2.b - don't obsolete the interfaces.)

The advantages/disadvantages are basically flipped from Option 1 above.

Thoughts

Following Option 2 is what we would traditionally do. It is the "safer" and more traveled road. Do we think Option 1 has enough advantages to try to blaze a new trail here?

cc @davidfowl @Tratcher @halter73

@Tratcher
Copy link
Member

Tratcher commented Nov 19, 2021

Option 2 would cause significant public API breaks up stack and is not DI friendly. Option 1 is much less invasive if you can make it work. If you find Option 1 to be insufficient or problematic after one or more releases, you can still fall back to Option 2.

@eerhardt
Copy link
Member

and is not DI friendly.

Can you elaborate? Why is an abstract base class "not DI friendly" vs. an interface?

@Tratcher
Copy link
Member

and is not DI friendly.

Can you elaborate? Why is an abstract base class "not DI friendly" vs. an interface?

I'd overlooked MemoryCacheBase, yes that would work for DI.

I still think DIMs are better for being less disruptive to the ecosystem. We've used them successfully in aspnetcore, though we didn't have the netstandard limitation.

@eerhardt eerhardt modified the milestones: 7.0.0, Future Jul 19, 2022
@fowl2
Copy link

fowl2 commented Dec 8, 2022

There's also the possibility of:

namespace Microsoft.Extensions.Caching.Memory;

+  public interface IMemoryCache2 : IMemoryCache
+  {
+      void Clear();
+  }

-  public class MemoryCache : IMemoryCache
+  public class MemoryCache : IMemoryCache, IMemoryCache2

With the option to obsolete IMemoryCache. A little inelegant - having to register multiple interfaces in DI, but backwards compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Primitives
Projects
None yet
Development

No branches or pull requests

5 participants