adobe / aem-core-wcm-components

Standardized components to build websites with AEM.
https://docs.adobe.com/content/help/en/experience-manager-core-components/using/introduction.html
Apache License 2.0
737 stars 745 forks source link

Do not build `Component` model in HTL if current component implements `Component` #1254

Open ky940819 opened 3 years ago

ky940819 commented 3 years ago

Is your feature request related to a problem? Please describe. When a component model implements the Component interface, then the component ID should be available directly from the component's model. There is no need to create (within the component HTL) an additional sling model for Component. Doing so causes extra unnecessary work, and leaves open the possibility that the component model may override the #getID() method, and that override would not be honoured by the HTL unless the component model also marked Component interface in the model adapters.

Describe the solution you'd like Remove the data-sly-use.component="..." from component HTL where there already exists a model that implements the Component interface.

e.g.:

<nav data-sly-use.breadcrumb="com.adobe.cq.wcm.core.components.models.Breadcrumb"
     data-sly-use.component="com.adobe.cq.wcm.core.components.models.Component"
     data-sly-use.template="core/wcm/components/commons/v1/templates.html"
     id="${component.id}"  ...>
    ...
</nav>

vs:

<nav data-sly-use.breadcrumb="com.adobe.cq.wcm.core.components.models.Breadcrumb"
     data-sly-use.template="core/wcm/components/commons/v1/templates.html"
     id="${breadcrumb.id}"  ...>
    ...
</nav>

Are there alternatives? Resolving #1061 would achieve the same outcome by ensuring that the data-sly-use.component="com.adobe.cq.wcm.core.components.models.Component" resolves the same component model, and thus any override to the ID would be honoured; However, it does not resolve the unnecessary overhead of constructing the model twice under different interfaces.

I believe that both the changes suggested in this ticket, as well as those in #1061 should be implemented so as to eliminate that overhead, while ensuring that regardless of how you get the component (adapting to the Component model interface, or the specific component interface that extends the Component interface), the results are the same.

For clarity, this should be the promise that is made (not real java)

String componentID = getModel(resource, Component.class).getID();
String breadcrumbID =  ((Component) getModel(resource, Breadcrumb.class)).getID();
Assertions.assertEquals(componentID, breadcrumbID);

// or more generally, being equivalent, not necessarily same.
Assertions.assertEquals(getModel(resource, Component.class), ((Component) getModel(resource, Breadcrumb.class)));
vladbailescu commented 3 years ago

One of the reasons we used data-sly-use.component="com.adobe.cq.wcm.core.components.models.Component" was that getId was introduced only recently and might not be implemented by customer's overriding models.

ky940819 commented 3 years ago

Is that something this project concerns its self with? Other model changes are made that require updates to the HTL to prevent them from breaking. Unless people copy the entire HTL over to their override components then components will frequently break on update.

vladbailescu commented 3 years ago

@ky940819 , we try to be good citizens. We might miss something but generally we want customers to feel safe to upgrade the Core Components version at all times. We collect known cases at https://github.com/adobe/aem-core-wcm-components/wiki/versioning-policies, these inform us when we need to create a new version of a specific component (because it's not backwards compatible).

We appreciate feedback there as well, if you know case we might have missed.