adobe / adobe/aem-core-wcm-components
[Architecture] Allow Core Components to be more effectively extended
- Dominant language
- Java
- Stars
- 795
- Forks
- 775
- Avg merge
- 3d 22h
- Merged PRs (30d)
- 8
Description
At Bounteous we were considering modifying the behavior of the Language Navigation to make one small changes.
Given this scenario:
- /content/site/en/lvl1/lvl2
- /content/site/fr/lvl1
WCM Core functionality:
- FR link on /content/site/en/lvl1.html goes to /content/site/fr/lvl1.html (same page)
- FR link on /content/site/en/lvl1/lvl2.html goes to /content/site/fr.html (home page)
Desired functionality:
- FR link on /content/site/en/lvl1.html goes to /content/site/fr/lvl1.html (same page)
- FR link on /content/site/en/lvl1/lvl2.html goes to /content/site/fr/lvl1.html (closest parent page)
Now I know we could submit a PR to WCM Core, and add a dialog option to change this behavior, but that would require us to then wait for the next WCM Core release. Also, not every developer in the wild is looking to contribute back to WCM Core (for various reasons) - instead they want to *leverage* the tool.
The challenge with the current implementation and extension pattern is that to make this very slight change in component functionality a developer is basically forced to rewrite the entire sling model and dependency POJOs as custom code in the client codebase.
Before I discuss the challenges, let's discuss what this *could* look like if sling models were not hidden impl classes. Note that this is not a discussion on the merits of hidden model impls and the proxy extension pattern, which we've debated on #466. This is simply to show the difference of how simple this *could* be to implement versus how difficult it is to do so at current.
**Sunshine and bunnies solution**
If `LanguageNavigationImpl.class` was not hidden, custom development of a sling model extending this model could simply `@Override` the `getLocalizedPage()` function (it would need to be `protected` instead of `private`) to return the desired page path. Everything else would remain as-is. Clean, simple, done.
**Actual solution (given current constraints)**
With that out of the way, let's consider the "real" solution that can be achieved given the current patterns in WCM Core.
Since the developer is required to use the sling proxy extension pattern, the developer is required to rewrite the logic to calculate the language links in their own implementation class. In short, they must basically rewrite the entire `LanguageNavigationImpl` class *or* use the proxied super class to first `getItems()` and then change that `List` into a completely new list `NavigationItem` objects (gaining some minimal leverage at best). Even if we go with that pattern, however, it now becomes even more challenging.
In overwriting `getItems()`, the developer is now forced to also overwrite the following line of code:
```
pages.add(new LanguageNavigationItemImpl(page, active, request, level, children, title));
```
However, `LanguageNavigationItemImpl` is also a hidden class, as is it's parent class `NavigationItemImpl`, as is it's parent class `PageListItemImpl`. Because these are not sling models, there is no way to leverage the work WCM Core has done in these classes. Rather, the developer must recode all of these classes into their own codebase in order to be able to build out the new `List` to be returned by their proxy extension of `LanguageNavigation`.
In the end, WCM Core requires the developer to rewrite `LanguageNavigationImpl`, `LanguageNavigationItemImpl`, `NavigationItemImpl`, and `PageListItemImpl` in order to effect this simple change of logic in calculating the language link path. Not only does the developer get no leverage from the WCM Core `languagenavigation` component, but the dev is also required to copy and paste the other `*ItemImpl` classes to their codebase with the possibility of them diverging over time from WCM Core where these classes are used also in other components.
**Now for potential solutions:**
Solution 1: Add `Page getLocalizedPage(Page page, Page languageRoot)` to the public interface of `LanguageNavigation`. It feels a little convoluted, but would be a simple solution. It would solve the issue of not being able to access `LanguageNavigationItemImpl`, `NavigationItemImpl`, and `PageListItemImpl` at least for this particular scenario, because the developer would not need to overwrite `getItems()`.
Solution 2: Update `LanguageNavigationItemImpl`, `NavigationItemImpl`, and `PageListItemImpl`, which are not sling models, to be public classes. At least then when the proxy extension of `LanugageNavigationImpl` overwrites `getItems()` it can leverage these classes without overwriting them. Even if we go with Solution 1, it might be worth considering this as well since its only a matter of time before another scenario comes up that needs access to these classes.
Solution 3: My prefered solution (as discussed in #466) is still to make all WCM Core model classes exported so they can be directly extended. It doesn't seem many people agree with me on that though :).
Thoughts?
Contributor guide
Research direction
The issue names LanguageNavigationImpl, LanguageNavigationItemImpl, NavigationItemImpl, and PageListItemImpl, plus LanguageNavigation#getItems() and getLocalizedPage(). Read those extension points and the discussion around Solutions 1–3, including #466; before coding, confirm which API change is wanted and define tests and acceptance criteria.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend-api-design, developer-experience
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100