Skip to content
This repository has been archived by the owner on Nov 29, 2018. It is now read-only.

Remove dependency on Microsoft.AspNetCore.Hosting.Abstractions #290

Closed
djanosik opened this issue Sep 30, 2016 · 15 comments
Closed

Remove dependency on Microsoft.AspNetCore.Hosting.Abstractions #290

djanosik opened this issue Sep 30, 2016 · 15 comments

Comments

@djanosik
Copy link

Can you remove the dependency on Microsoft.AspNetCore.Hosting.Abstractions from the Microsoft.Extensions.Localization package? We wanted to use the new localization in our framework, but we don't want any dependency on ASP.NET Core.

@Eilon
Copy link
Member

Eilon commented Sep 30, 2016

Oops, you are correct. This must have been an oversight on our behalf. Unfortunately, changing it at this time is a breaking change.

But it's something we can certainly fix in a new major version.

It appears that the dependency is there because it uses IHostingEnvironment to get the application name. We should have instead defined a new interface, e.g. IApplicationName, which isn't part of ASP.NET Core. This localization library would depend only on that new interface. Then we'd have a new implementation of that, e.g. AspNetApplicationName : IApplicationName, which uses IHostingEnvironment to get that data.

@Eilon
Copy link
Member

Eilon commented Sep 30, 2016

cc @DamianEdwards

@djanosik
Copy link
Author

djanosik commented Sep 30, 2016

It makes no sense. Just make sure the location parameter is not null.

@Eilon
Copy link
Member

Eilon commented Sep 30, 2016

@djanosik sorry, not sure I understand your comment.

@hishamco
Copy link
Contributor

hishamco commented Sep 30, 2016

AFAIK the new Localization APIs are designed for ASP.NET Core. If we remove the dependency on Microsoft.AspNetCore.Hosting.Abstractions you can use the localization abstractions & some of the implementation APIs, because you miss the Microsoft.AspNetCore.Localization which have a rich APIs to get the culture information from different providers on top of the xxxStringLocalizerFactory and xxxStringLocalizer, this is main package that developers may need to localize their apps, unless you will build this from scratch

@djanosik
Copy link
Author

djanosik commented Oct 1, 2016

@Eilon It should not be responsibility of the ResourceManagerStringLocalizerFactory to get the name of application. The location parameter in the Create(string baseName, string location) method should never be null.

@atuzovic
Copy link

atuzovic commented Dec 11, 2016

@Eilon - So new version is out (1.1.0) but dependency is still there.
Is it going to be decoupled in the future?

@Eilon Eilon added this to the 1.2.0 milestone Dec 12, 2016
@Eilon
Copy link
Member

Eilon commented Dec 12, 2016

@atuzovic yeah we'll look at this for the next milestone. It's definitely something we want to do.

@DamianEdwards
Copy link
Member

We can't remove dependencies until a major release as it's a breaking change so you won't see this until 2.0

@hishamco
Copy link
Contributor

@Eilon It should not be responsibility of the ResourceManagerStringLocalizerFactory to get the name of application. The location parameter in the Create(string baseName, string location) method should never be null.

@djanosik I don't think the location should be never be null because there 're many cases where the location is not required, for example using EF as resources source, so that's why I proposed to redesign IStringLocalizerFactory in #302, I have some ideas to remove the dependency by redesign that interface in one go. Hopefully I'll update that issue soon, or may be I will write down a proposal PR

@ryanbrandenburg
Copy link
Contributor

My plan of attack here is to create IApplicationNameProvider in Microsoft.Extensions.Localization.Abstractions and HostingEnvironmentApplicationNameProvider in Microsoft.AspNetCore.Mvc. I think Microsoft.AspNetCore.Mvc is the best location for the provider because it already has both Microsoft.AspNetCore.Hosting.Abstractions and Microsoft.Extensions.Localization.Abstractions, so we don't need to bring a new dependency to anything.

@Eilon
Copy link
Member

Eilon commented Jan 27, 2017

Sounds good to me.

@hishamco
Copy link
Contributor

hishamco commented Feb 3, 2017

@ryanbrandenburg this should be closed after the commit 65e3af9

@Eilon
Copy link
Member

Eilon commented Feb 3, 2017

@ryanbrandenburg can you post an announcement as well? Need to mention the dependency that we removed and the small behavior breaking change of the API.

@ryanbrandenburg
Copy link
Contributor

An announcement was made for this breaking change in aspnet/Announcements#218.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants