Pager should inherits from LocalizableComponent

[hishamco]

This addresses the issues described by @sbwalker here

I come up with three solutions but the easy and the better one is to trim the generic type symbol and let Pager inherits from LocalizableComponent

[hishamco]

@leigh-pointer can you confirm that the PR fixes the issue

[sbwalker]

@hishamco I like the solution of having Pager inherit from LocalizableComponent, as this is consistent with the approach used by the other components in the Controls folder.

That being said, it appears that the modifications are a breaking change for any existing components created by third parties which already inherit from LocalizableComponent. Specifically I am referring to the method name change from Localize() to LocalizeResource() - which required modifications to all other components in the Controls folder. Since Oqtane is a framework which is intended to be extended by developers, we try to avoid breaking changes if possible. And in this case it appears that renaming the method name is not necessary - it would be possible for Pager to inherit from LocalizableComponent without introducing a breaking change.

I am also curious about the logic in the CreateLocalizer() method. It appears to reference a few "magic strings" which makes the logic difficult to understand. Can you please explain the scenarios where a type name may contain a "`1" and how this logic will handle it? The code also appears to reference "Oqtane." as a hardcoded prefix which means that LocalizableComponent can only be used by components which are internal to the framework? Is there a reason for this? In most other cases in Oqtane you can create extensions using your own namespace and assembly name.

[hishamco]

You are right about breaking changes, if you just test it will be fine. At the beginning I though such API is used internally, but while it's protected it could be used by others

Can you please explain the scenarios where a type name may contain a "`1"

If you look to any generic type, .NET append "1" suffix to the name, I tried to trim this, so the localizer can look for "Pager" instead of "Pager1"

The code also appears to reference "Oqtane." as a hardcoded prefix which means that LocalizableComponent can only be used by components which are internal to the framework?

The idea here is to specify the base name which can be generated from the control namespace, but while the Oqtane is the root for the resources, it's good to remove it otherwise the search localization will be wrong

In most other cases in Oqtane you can create extensions using your own namespace and assembly name.

For that we may need another test, it might be a problematic if we inherits from LocalizableComponent outside Oqtane project. @leigh-pointer could you please test this with one of your custom modules?

[hishamco]

I will revert the breaking changes, unless the upcoimng version is a major one

[sbwalker]

@hishamco yes please revert the breaking changes - the next version will not be major, it will be 3.1.3... and even if it was major, we try to avoid breaking changes in Oqtane unless they are absolutely required (which is not the case here).

[hishamco]

I just revert the breaking change, but we might consider a proper naming in the upcoming major release, to make our API clear enough

Now, I will try to address the assembly and namespace issue

[hishamco]

@sbwalker It's ready from my side, I fixed the magic string issue by trimming the common prefix between the assembly and control type names, to get the resource properly

[sbwalker]

@hishamco PR #2232 created a conflict in SharedResources.resx - are you able to resolve?

Another observation I have about this PR is that in .NET, the backtick notation "`" is used to identify generic type parameters - however there may be more than one generic type so the suffix is not always "`1" - it can be "`2" or "`3", etc... So the logic which looks for "`1" does not seem to handle all scenarios?

[hishamco]

are you able to resolve?

Sure

the suffix is not always "1" - it can be "2" or "3", etc... So the logic which looks for "1" does not seem to handle all scenarios?

I never seen a generic type with other than `1 did you see anything?

[hishamco]

You are right Shaun, let me change this

[sbwalker]

@hishamco I merged this PR which allowed me to review the run-time characteristics of the code and I have some additional comments...

First, it is important to clarify the purpose of LocalizableComponent. It is my understanding that this base class was created for a specific purpose - to allow some of the generic/shared components in \Modules\Controls such as Label, ActionLink, etc... to be localized within a specific module component. For example, in a Module's Edit.razor component you might have multiple instances of the Label component - however you want to be able to manage all of the localization resources for that module component in the same RESX file. So the LocalizableComponent utilizes the ModuleState.ModuleType to get the module component's type so that it can locate the resources. Note that this is only applicable to cases where a generic/shared component has dynamic content which can be defined by the developer. If a generic/shared component has static content which never changes when it is instantiated, then it does NOT fit this use case - it should instead be localized using the standard .NET Core pattern of using an IStringLocalizer. A good example is that the PermissionGrid is a generic/shared component in \Modules\Controls however it does not inherit from LocalizableComponent because it only contains static text elements. I am explaining all of this in detail because in a framework it is very important to have clarity around specific use cases, and ensure that features are focused on a single purpose - as when you start overloading their meaning it becomes very confusing.

So based on what I just described above, the Pager scenario which was the focus of #2209 and this PR does NOT align with the purpose of LocalizableComponent. I am saying this because the content to be localized in the Pager component is static. And it is actually very clear from the changeset that it does not fit the use case because you had to create an entirely different code path in LocalizableComponent to handle this scenario. So I have to apologize for my comments earlier in this thread, as the direction I provided was not valid. It would be better to NOT use LocalizableComponent in this case.

Which brings us back to the original question... what is the recommended approach from Microsoft when you cannot use a standard IStringLocalizer due to the type having generic parameters?

[hishamco]

A good example is that the PermissionGrid is a generic/shared component in \Modules\Controls however it does not inherit from LocalizableComponent because

I was thinking about it when I pushed the PR, I will create a PR for it ASAP

[hishamco]

Thanks for your explaination @sbwalker, the LocalizableComponent built to let any component support localization as first class citizen. It allow us to provide a resource key that we can look for from Resx file such. Nothing but that doesn't mean that we can't used it for any other components including Pager.

The main difference here as you mentioned above some have dynamic resources and some have static resource, but behind the scene we are using IStringLocalizer

In case you are suggest to let LocalizableComponent usd for dynamic resource, I may revert the recent changes and fix the page localization in code section

[sbwalker]

@hishamco correct - I am saying that LocalizableComponenent should NOT be used for the Pager. Instead Pager should use a standard static localization approach. My question was: What does Microsoft recommend in these situations? Since IStringLocalizer is the standard approach for most component localization scenarios - what does Microsoft suggest for dealing with generic type parameters? I have a hard time believing this is such an obscure scenario that other developers have not encountered it in the past... so there should be a standard answer. And if there is not a standard answer, should this be considered an issue with .NET Core Localization (ie. why is support not provided)?

[hishamco]

My question was: What does Microsoft recommend in these situations?

Frankly I don't know, that is why we struggled to use the current approach. I remembered I blogged about Localization & Generics which is referenced in AspNetCore docs

[hishamco]

And if there is not a standard answer, should this be considered an issue with .NET Core Localization (ie. why is support not provided)?

After almost - if not all - AspNetCore repos has merged I can't add any features on the localization module :(

[hishamco]

So, can we use code behind in case the current approach is not fit nicely?