netcentric / aem-htl-style-guide Goto Github PK

Hi guys,
I'm working on AEM IntelliJ Plugin which supports HTL and got issue report about variables resolving.
In such code:

variables are unresolved because in the plugin I assume that (according to HTL specification) identifiers scope for data-sly-use, data-sly-test, data-sly-set and data-sly-unwrap is specified as:

Scope: The identifier set by the * block element is global to the script and can be used anywhere after its declaration.

For me, the "after its declaration" phrase means that usage before the declaration is disallowed. Maybe I'm wrong with interpretation.

My question is: what is the reason for adding rule 4.7 to the style guide? In my opinion, code is hard to read when declarations are specified after actual usages and I would even add rule exactly opposite to the current 4.7 - to put HTL blocks always before any other attributes. Is there any technical reason for the current state or it was introduced for better code organization and putting standard attributes before HTL blocks was a random decision or a matter of taste? Maybe it's worth to change it to make code cleaner and follow HTL spec?

I'm not sure if I correctly understand rule 4.2. What does "root element" mean? Is it just top level element:

<div> <!-- this is root element -->
    <span>test</span>
</div>
<section> <!-- this is root element -->
    lorem ipsum
</section>

or first top level element in file?

<div> <!-- this is root element -->
    <span>test</span>
</div>
<section> <!-- this is NOT root element -->
    lorem ipsum
</section>

What is the reason in change of mind to now prefer <sly></sly> over <sly/>?
I'd still recommend to using the self-closing tag.

5f3eaa4

It says there

This rule only applies if you are using AEM 6.0 or when you add Client libraries to your page.
But this rule has nothing to do with client libraries at all. Probably the sentence can just be removed.

I suggest the following improvements to make it easier to reorganise code (moving sightly instructions around, etc.) and also to improve readability. These suggestions come from my firsthand experience on working with Sightly on a large AEM project. There you often need to reorganise code and change control flow, which is way easier, when these things are separate tags instead of sticking everything on one HTML tag. Also, readability gets really bad with the currently suggested rules. As an example, we have components on our project with several use classes on the root element, plus we also have a test statement on the root element and finally we also have some special CSS classes on this root element which are generated by the use classes.

4.2 - Do not put the use tags on the regular root element. Instead, i would put them in a separate sly tag at the top of the file. Also, i would put a separate sly tag for each use js/java class. In that way it is easier to change the application structure without having to move attributes around, creating tags, etc.:

<sly data-sly-use.teaser="com.example.FirstComponent" />
<sly data-sly-use.somethingElse="com.example.SecondComponent" />
<section class="teaser">
    <h3>${teaser.title}</h3>
</section>

4.7 and 4.8 - I suggest to actually put control statements on separate sly tags to disentangle the control logic from the actually output HTML. In that way it is easier to change the control / flow of the execution without having to move attributes around:

<sly data-sly-test="${teaser.text}">
<p class="teaser__text"></p>
</sly>

As the identifiers in Sightly (at least the one for the template identifier) are internally converted to lowercase, they should only be defined in lowercase in the first place. Otherwise one might run into collisions when defining a template named "myTemplate" and "mytemplate" in the same script, because to Sightly those identifiers are the same.
Reason is primarily that Sightly logs everything with the internal (lowercased) identifiers and finding the appropriate location in the Sightly script is much easier, if the identifier is also used in lower case in the script.

Since the scope of everything being included via data-sly-use is always global to the current Sightly file (http://docs.adobe.com/docs/en/aem/6-0/develop/sightly/block-statements.html#use) it would be good to use data-sly-use only at the root element. That way one can see at first glance which things are imported. Otherwise the same e.g. template might be imported multiple times, which leads to

• a warning from the org.apache.sling.scripting.sightly.impl.compiler.util.GlobalShadowChecker
• performance degradation as things are initialized multiple times

The following example doesn't comply with §4.8 (Always use existing HTML elements for your block statements if possible).

<sly data-sly-use.teaser="com.example.TeaserComponent">
    <div class="teaser">
        <a class="teaser__link" href="${teaser.link}"></a>
    </div>
</sly>

Because as far as I understand the style guide, §4.8 would request it to be written as follows:

<div class="teaser" data-sly-use.teaser="com.example.TeaserComponent">
    <a class="teaser__link" href="${teaser.link}"></a>
</div>

Maybe a better example would be something like the following:

<!--/* Bad */-->
<p data-sly-use.foo="Foo">${foo.one}</p>
<p>${foo.two}</p>

<!--/* Good */-->
<sly data-sly-use.foo="Foo">
    <p>${foo.one}</p>
    <p>${foo.two}</p>
</sly>

HTL Spec was updated in June 2018 to support variable setting via data-sly-set (https://github.com/adobe/htl-spec/blob/1.4/SPECIFICATION.md#2212-set) and available in AEM 6.5 (https://docs.adobe.com/content/help/en/experience-manager-65/release-notes/sites.html), AEM 6.3 SP3 and AEM 6.4 SP1 (https://github.com/adobe/htl-spec).

This potentially means we should now recommend the usage of data-sly-set when possible.

We should probably also mention that setting up variables by abusing the data-sly-test.identifier pattern for cases other than test blocks is to be avoided when possible.(https://github.com/Netcentric/aem-htl-style-guide#4.5).

Basically this says that Sightly is not accepting valid HTML4/5. This would be a bug in HTL/Sightly which should be reported upstream. We need to figure out which version has been affected from this bug.

Currently the Sightly Compiler will never remove any blank lines from the HTML (see https://issues.apache.org/jira/browse/SLING-4443).
That may lead to problems because a code like this

<sly data-sly-use="..." data-sly-unwrap />
<!DOCTYPE>

will lead to an output like this

<!DOCTYPE>

There is an empty first line being rendered by the Sightly script.
One solution would be to always have the end tag of data-sly-unwrap elements at the beginning of the new line, so you would rather write it like this

<sly data-sly-use="..." data-sly-unwrap 
/><!DOCTYPE>

It is often hard to distinguish between the different types of identifiers used within a Sightly file.
The following types of identifiers should be distinguishable by only looking the identifier itself:

• JavaScript/Java object mapped via data-sly-use
• Template library initialized via data-sly-use
• Template name defined via data-sly-template

I propose to use different prefixes/suffixes for those types.
Those prefixes/suffixes will also prevent any name clashing will other predefined global objects: http://docs.adobe.com/docs/en/aem/6-0/develop/sightly/global-objects.html

It seems that Adobe copied a lot of our style guide for their own wiki

https://github.com/adobe/aem-core-wcm-components/wiki/htl-style-guide

Our style guide is a bit more strict in some ways...

i.e.:

Always use HTL comments.

Normal HTML comments get rendered to the final markup. To keep the DOM clean, always use HTL comments over normal HTML comments.

vs

Prefer HTL Comments Over HTML Comments
HTML comments are rendered in the final markup, so unless there is a specific need, HTL comments are always preferred.

this, I find okay.

But we could probably adopt some of their recommendations:
i.e.:

Drop Method Prefixes When Accessing Properties from Java Getter Functions

I think it'd be good to do a revision of our recommendations.

I will open a few PR's and will await your comments.

@raohmaru Pointed to me that Github already adds links when using the Heading syntax.
To keep the markdown simple, it would probably be good to change our docs to use this functionality directly instead of creating the links manually as we do right now.

It should be noted however, that this might make our docs less portable if we want to migrate out.

Instead of

<sly data-sly-test="${!teaser.active}" data-sly-unwrap> ...</sly>

you could

<sly data-sly-unwrap="${!teaser.active}"> ...</sly>

If the resourceType option is left out and the referenced resource does not exist (i.e. for pages being created based on an old template that didn't have that resource) Sightly will throw a org.apache.sling.api.resource.ResourceNotFoundException for the data-sly-resource statement. To prevent that the resourceType should be given wherever possible. That way the Sightly script will still render fine and the resource will be automatically created the first time the edit dialog is submitted by the author. That way pages do not have to be manually migrated/recreated once a new component is added.

The only cases where this is not possible are

• directly within a parsys (where arbitrary resourceTypes are supported). There is never a problem with these includes, as only existing child resources should lead to rendering a component (non-existing resources are never used with such a data-sly-resource)
• in reference components referencing other content as is (i.e. without possibility to edit). In that case the path is given by the editor and throwing an exception in case an inexisting path is used there is fine for that as well.