[ENH] ContainerBase Should Not Inherit From ThemeBase

[iJungleboy]

Oqtane Info

Version - 6.0

Describe the enhancement

At the moment many Oqtane controls - including containers and themes - MUST inherit from an Oqtane-specific base class.
This is because certain properties such as Name will only be known to Oqtane if the implementation inherits from these base classes.

This also adds a lot of baggage to each control and container. For example:

1. every inheriting component incl. container will run code to retrieve the (async) correct resources - whether it needs them or not
2. every inheriting component incl. container has a fairly large (and IMHO confusing) standard API which I would like to avoid. These are from ThemeBase and incl. loads of overloads of NavigateUrl(...) each with minor variations
3. Every such control also gets things injected and parameters passed in, which are often never used - not ideal for performance.

BUT: my understanding of good architecture prefers composition,
which is what I would like to encourage in various docs. But as of now, this is not possible.

My recommendation is that we promote Composition. This requires the following changes - for example for containers:

1. An interface like IContainer - or we could enhance the existing IContainerControl
2. This would require the really necessary parameters - such as Name

Note: ATM there is something similar on the IThemeControl, but this should not be used

• it has additional properties such as PaneNames which are simply not necessary on most components or even Containers.
• it also has a Resources which again may not be relevant to the implementation of most controls, so allowing that on a separate optional interface like IHasResources or something would make much more sense.

Anything else?

Would really appreciate if we could change this, to promote good practices and ensure long term stability of the API without cluttering the code.
I would also contribute the change if it's approved.

[sbwalker]

Oqtane already utilizes interfaces. For example, the IModuleControl interface defines the core properties which a module component can implement. However due to the fact that interfaces are immutable it is essential to create an abstraction so that third party developers do not take a direct dependency on an interface (as otherwise, anytime the interface changed it would result in a breaking change). The way this is accomplished is by including a base class (ie. ModuleBase) which implements the interface and acts as an abstraction layer. This way a module developer can take a dependency on ModuleBase and not need to worry if the interface changes. This is a well accepted architectural approach when creating a framework/platform (which is not common when building applications as you do not need to worry about compatibility). The base class also provides some common utility methods and events to provide standard, simplified methods for interacting with the framework.

[sbwalker]

I think what you are actually trying to report in this issue is that ContainerBase has some inconsistencies. It implements IContainerControl (which is the standard pattern I described above for ensuring backward compatibility) however IContainerControl does not have any properties defined. And ContainerBase inherits from ThemeBase - which means it inherits a bunch of functionality related to themes which is not 100% applicable to containers. This was implemented 4+ years ago and I believe the rationale at the time was that there was enough overlap in theme and container functionality that it did not make sense for containers to have their own distinct implementation (which would essentially duplicate the majority of functionality in ThemeBase). However, as the framework evolved there has been more divergence between theme components and container components, so this rationale may no longer be accurate. So from a purist perspective it might make sense to consider a change... but from a practical perspective it depends on how disruptive it will be for existing themes (ie. from a compatibility perspective). This will require some investigation.

[iJungleboy]

I understand that and there is quite a lot going for this.

What I don't like is three things:

1. that simple things such as containers inherit from quite a heavy ThemeBase class which seems to have grown and has APIs which are actually not relevant at all - such as the Panes and also Thumbnail (though that could come in handy some day). It looks like the ThemeBase was developed for both the main Theme and used for components at the same time, resulting in some mixed APIs.
2. that it always does work such as resources processing, which is often not relevant (again because of inheriting the ThemeBase which feels wrong
3. that it - and many other controls - have a fairly random mix of "helper" APIs such as NavigateUrl, the overload signature of which can only be explained by history (gradually adding more variants).

What I think would be more elegant is a "pure" base class which only does the necessary minimum and no overhead or additional APIs.

Since you have the (absolutely justified) concern for the base class which may need to be enhanced by the platform itself, and the current ContainerBase already has the APIs (so it cannot be down-converted to a lightweight base class),
maybe we could find a middle ground for things such as containers, modules, theme-controls etc.
An idea could be to also have bare-metal base classes, such as:

• ContainerBareMetalBase
• ContainerRawBase
• ...

Something like this - and equivalent raw/bare-metal components.

[sbwalker]

1. I already noted the inconsistency in ContainerBase above and plan to investigate further
2. If the Resources collection is empty, no work is actually done - so this is still very light-weight from a run-time perspective. It is also important to note that every Blazor component implements the OnAfterRenderAsync life cycle event.
3. The number of helper methods has increased over time to provide support for various scenarios - they are provided so that developers do not have to figure out how to construct a valid url from scratch (which can be challenging based on the route conventions)

I am not in favor of changing the architecture for the sake of "purity". The Oqtane Philosophy (https://www.oqtane.org/blog/!/20/oqtane-philosophy) outlines that we are trying to use "Practical Engineering" and "Consistency" so that we can avoid these types of subjective topics. However, it should be noted that you should be able to create your own custom base classes, as long as you implement the required interfaces.

[iJungleboy]

@sbwalker Creating a lightweight Container was my intent, but it fails.

I can implement the IContainerControl, but Oqtane will try to cast it to an IThemeControl when loading here:

oqtane.framework/Oqtane.Server/Repository/ThemeRepository.cs

Lines 304 to 313 in c819058

	var containerobject = Activator.CreateInstance(containertype) as IThemeControl;
	theme.Containers.Add(
	new ThemeControl
	{
	TypeName = containertype.FullName + ", " + themeControlType.Assembly.GetName().Name,
	Name = (string.IsNullOrEmpty(containerobject.Name)) ? Utilities.GetTypeNameLastSegment(containertype.FullName, 0) : containerobject.Name,
	Thumbnail = containerobject.Thumbnail,
	Panes = ""
	}
	);

There it will ask for the Name (of IThemeControl) - without null-check, and Thumbnail.

I hesitate to implement IThemeControl even if I could get it to work, because IMHO implementing IContainerControl should be all it needs.

I would suggest

1. Expand IContainerControl to have a Name, and possibly a Thumbnail if you want to implement a kind of preview some day.
2. Change the code to not cast to IThemeControl when loading containers.

Side Note: When I'm reviewing the code my impression is that Container Resources are probably never loaded. My guess is this is an oversight, but then again may be a reasonable restriction to reduce complexity. Either way I suggest that Resources should not be part of IContainerControl, or we should make sure that container resources do get loaded.

[sbwalker]

I was reviewing this yesterday. The only IThemeControl interface property which is not applicable to container components is Panes... all of the other properties are applicable:

Name - provides "friendly" name for a container component
Thumbnail - intended to provide a url for displaying an image thumbnail for a container component (not implemented currently)
List Resources - provides a collection of resources (CSS,JS) for a specific container component

If ContainerBase did not inherit from ThemeBase it would need to support all of the same default services, state objects, and helper methods which exist in ThemeBase - so basically the only difference is that it would not include public virtual string Panes { get; set; }.

[iJungleboy]

I guess this makes sense.

From my point of view I would prefer if IContainerControl would require these, and would allow anybody who implements the interface create clean containers.

ATM the interface is there, but it doesn't serve a purpose, since implementing it won't actually result in the container being loaded.