Skip to content

Shared framework package introduces invisible dependencies through namespace pollution #3264

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

Closed
poke opened this issue Jun 25, 2018 · 2 comments

Comments

@poke
Copy link
Contributor

poke commented Jun 25, 2018

I was just upgrading a project to use the .App shared framework instead of individual package references when I noticed a problem there. In my code, I have been using a ClaimsPrincipal.FindFirstValue extension method which I added to my project. The same extension method is also part of Identity in Microsoft.Extensions.Identity.Core.

By switching to the shared framework, this caused my project to throw ambigous call compiler errors since both (identical) extension methods are in the same namespace. Not a big problem as I can just remove my own implementation. However, doing so will make me use the one in Identity.Core and as such add an unwanted hard reference to that assembly that previously did not exist.

Now, you could argue that the shared framework will make this a non-issue since the package is available on the machine anyway. And in a way I agree with that. It’s also not that the package itself has a lot of other stuff that would cause additional problems. But it’s still another dependency my project has, and I dislike this outcome on principle. It’s not an uncommon practice within ASP.NET Core that packages have extension methods for types outside of their own namespaces. So this is really a kind of namespace pollution that has the bad side effect of introducing dependencies invisibly through the shared framework.

I do realize that there isn’t much we can do about this now, but I am really unhappy that this is another complication the shared framework causes me. I’m currently considering to completely pass on the shared framework in all my projects because of all the issues (or at least questions) its magic raises (e.g. this). And I think that’s a real shame since the intention was really good.

I don’t know how I would ideally like to see this issue get resolved. I guess I would be happy if this problem (among those others) would be considered for future iterations of the shared framework so using it becomes less problematic. Maybe there could be some opt-out mechanism for packages or something.

@Eilon
Copy link
Contributor

Eilon commented Jul 13, 2018

Hi @poke , this is a very interesting topic indeed.

Regarding this:

It’s not an uncommon practice within ASP.NET Core that packages have extension methods for types outside of their own namespaces.

It's actually supposed to be a discouraged and extremely uncommon practice within ASP.NET/EF Core to have public extension methods for types outside of their namespace for things that are not specific to their own scenarios. The most common case of this is extension methods for registering services and for registering middleware, but I think most consider those to be reasonable and expected. But the number of public extension methods for "arbitrary" types (e.g. collections, primitives, etc.) is intended to be as close to zero as possible.

The case of extension methods for ClaimsPrincipal is a bit more of a gray area to me: it's not exactly within the scope of Identity to have such a method, but it's very common for developers using Identity to need this particular method.

But, as you say, with metapackages and whatnot, it's a lot easier to be affected by something you weren't expecting. I also don't have a great solution to this. Opt-out for packages is quite a tricky thing because it's not really clear what effects that could/should have.

I think being more vigilant about public extension methods is a fairly straightforward thing we can apply to future work that should mitigate most of this problem.

@Eilon Eilon added this to the Discussions milestone Jul 13, 2018
@aspnet-hello
Copy link

We periodically close 'discussion' issues that have not been updated in a long period of time.

We apologize if this causes any inconvenience. We ask that if you are still encountering an issue, please log a new issue with updated information and we will investigate.

@aspnet-hello aspnet-hello removed this from the Discussions milestone Sep 24, 2018
@dotnet dotnet locked and limited conversation to collaborators Sep 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants