Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Factory should implement caching #50

Closed
lugospod opened this issue May 1, 2019 · 3 comments
Closed

Factory should implement caching #50

lugospod opened this issue May 1, 2019 · 3 comments

Comments

@lugospod
Copy link

lugospod commented May 1, 2019

Hi,

In the default implementation of IStringLocalizerFactory, they do IStringLocalizer caching (using ConcurentDictornary). According to feedback from the .NET CORE development team, if one decides to implement a custom IStringLocalizerFactory they should implement caching them self.

Implementation should cache localizers to prevent them from being created needlessly.

Unfortunately your test case is small controller so you can't reproduce this issues. In my case, where there is massive use of ISTringLocalizer, it is called many times.

Why was there a need to implement the JsonStringLocalizerFactory? As far as I can tell, you could have solved it all just by using the IStringLocalizer implementation and just keep using the Microsoft default IStringLocalizerFactory (that uses caching)

I will try to make it work without it in my scenario because I currently don't see the benefit of Factory customization. But please let me know if I am missing something...

tnx!
Luka

@AlexTeixeira
Copy link
Owner

Hi,

Seems that you're totally right.
I implemented it at beginning, I think because I didn't have the knowledge at the beginning of the project.

Removing the Factory is quite easy. The bid deal is to keep benchmark and test project working

@AlexTeixeira
Copy link
Owner

Hi again,

I make some changes and take your pull request #44 and #43 in the same time.

I keep the Factory only for the baseName. I don't know how to use the base name path without creation at specific factory.

I will create a pull request and set you as Reviewer to check if I did anything wrong

AlexTeixeira added a commit that referenced this issue May 1, 2019
AlexTeixeira pushed a commit that referenced this issue May 2, 2019
AlexTeixeira added a commit that referenced this issue May 2, 2019
@AlexTeixeira
Copy link
Owner

See version 2.1.0-preview-1

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

2 participants