adobe / htl-spec

HTML Template Language Specification
Apache License 2.0
280 stars 146 forks source link

data-sly-alias #25

Closed paul-bjorkstrand closed 6 years ago

paul-bjorkstrand commented 8 years ago

I want to propose a new block statement for HTL: data-sly-alias. With this, you could store an intermediate result of a long getter chain. Currently, you must use the full chain of getters when accessing a deeply nested value:

<div data-sly-use.object="SomeObject">
  <div>${object.child.grandChild.greatGrandChild.value1}</div>
  <div>${object.child.grandChild.greatGrandChild.value2}</div>
  <div>${object.child.grandChild.greatGrandChild.value3}</div>
</div>

With data-sly-alias it would be much more terse, by storing the intermediate evaluation of object.child.grandChild.greatGrandChild:

<div data-sly-use.object="SomeObject"
     data-sly-alias.greatGrandChild="${object.child.grandChild.greatGrandChild}">
  <div>${greatGrandChild.value1}</div>
  <div>${greatGrandChild.value2}</div>
  <div>${greatGrandChild.value3}</div>
</div>

Note: For a workaround that isn't a hack using data-sly-test, see my comment below

paul-bjorkstrand commented 8 years ago

An additional note, this also has the benefit that when you have a very long Java getter, it could be shortened, just the same as a long chain of getters:

<div>
  <div>${someObject.thatThingThatLooksLikeTheThingWeWantToHave.prop1}</div>
  <div>${someObject.thatThingThatLooksLikeTheThingWeWantToHave.prop2}</div>
  <div>${someObject.thatThingThatLooksLikeTheThingWeWantToHave.prop3}</div>
</div>

can turn into:

<div data-sly-alias.thingWeWant="${someObject.thatThingThatLooksLikeTheThingWeWantToHave}">
  <div>${thingWeWant.prop1}</div>
  <div>${thingWeWant.prop2}</div>
  <div>${thingWeWant.prop3}</div>
</div>
sljones1 commented 8 years ago

Currently you can use data-sly-test as a workaround, but this usage is very hacky. It works because data-sly-test doesn't test true or false, it tests truthy and falsy, which can lead to issues if you assume it is Boolean (ie. newer HTL users). For Example:

data-sly-test.greatGrandChild implies that ${greatGrandChild} will equal true, because we are testing it's existence. However

<div data-sly-use.object="SomeObject"
     data-sly-test.greatGrandChild="${object.child.grandChild.greatGrandChild}">
  <div data-sly-test="${greatGrandChild == true}">
        <p>Success</p>
  </div>
</div>

Doesn't compile, so greatGrandChild isn't a Boolean, it's an Object. @paul-bjorkstrand I feel data-sly-alias will help distinguish between assignment and actual testing for existence. I hope this is implemented in a new release 👍

paul-bjorkstrand commented 8 years ago

@sljones1 that was my thought as well. I am not super fond of using the data-sly-test workaround either. I used it once on a previous project, and confused the heck out of some junior devs, since they were wondering why I was 'testing' if an object was there, even though it was guaranteed to be there, in the model object.

adamcin commented 8 years ago

Should data-sly-alias simply be an alias for data-sly-test, or should it only test for non-null and ignore boolean evaluation?

sljones1 commented 8 years ago

@adamcin as long as AEM just continues to use truthy and falsy for data-sly-test, and don't decide to switch to true and false. But the odds are low they would do that, given how prevalent the assignment/test hack is.

karollewandowski commented 8 years ago

I think it's great idea, but I'm wondering if data-sly-alias would be a good name. "Alias" suggests that every reference to it would call entire expression and I assume it's not what we all think about. In my opinion better name would be data-sly-set (like in JSP), data-sly-assign, data-sly-value, data-sly-var or similar.

paul-bjorkstrand commented 8 years ago

@adamcin my vote would be for data-sly-alias to propagate nulls, meaning if greatGrandChild was null then

<div data-sly-use.object="SomeObject"
     data-sly-alias.greatGrandChild="${object.child.grandChild.greatGrandChild}">
  <div>${greatGrandChild.value1}</div>
  <div>${greatGrandChild.value2}</div>
  <div>${greatGrandChild.value3}</div>
</div>

would result in

<div>
  <div></div>
  <div></div>
  <div></div>
</div>

since HTL uses null safe/short circuiting.

@karollewandowski interesting thought. I could see opportunity for both (though if you design your backing model to not be idempotent, there is a special place in hell for you :wink:).

If the assumption is made (and we all know what happens when you assume), that a model's getters are idempotent, then an optimized implementation of data-sly-alias (storing the intermediate result) would be the same as a -set, -value, or -assign but that would be up to the implementer.

Using that assumption, the simplest implementation of data-sly-alias is where the short hand statement is replaced with the longer form, in the compiled script.

paul-bjorkstrand commented 8 years ago

A temporary workaround, not using the data-sly-test hack could be

setValue.js

use(function() {
  return this.value;
});
<div data-sly-use.object="SomeObject"
     data-sly-use.greatGrandChild="${'setValue.js' @ value=object.child.grandChild.greatGrandChild}">
  <div>${greatGrandChild.value1}</div>
  <div>${greatGrandChild.value2}</div>
  <div>${greatGrandChild.value3}</div>
</div>

Not quite as hacky, but more verbose than this proposal

karollewandowski commented 8 years ago

It's not only about idempotence - I've seen a lot of getXXX() methods containing complex and expensive logic (instead of just returning values assigned to fields). "Set" approach would be just safe and more efficient. I can't find reasonable cases for aliases.

paul-bjorkstrand commented 8 years ago

I have seen the same, and shudder when there is tons of logic in a getter.

Naming and implementation of the shortening is not what I wanted to get into in this request. That is why I opted for (and called it) aliasing. The runtime implementation wouldn't need to store it as a variable in its execution scope, all it needs to do is a literal replace during compile from HTL -> Java.

Don't get me wrong, I like your proposal for set. I was merely looking for the path of least resistance to getting something like this in, and the aliasing seemed to require the least amount of change. If set is easy enough, then I say to Adobe: Go For It!

If data-sly-set is what we end up with, I want to continue to point out my vote for preferred semantics, where it propagates null values, and all markup is retained (<sly> and data-sly-unwrap notwithstanding).

See https://github.com/Adobe-Marketing-Cloud/htl-spec/issues/25#issuecomment-249012447 for my propsal for the semantics.

karollewandowski commented 7 years ago

I think that's important case here is value scope. After some time of work with HTL I realized that its beauty is declarativeness. Simple alias/set with global scope (from declaration to end of file) would break it. I mean that there would be fragments like:

<div>
  ...
  <div>
    <sly data-sly-alias.street="${person.details.address.street}"></sly>
    ...
  </div>
  <span>${street}</span>
  ...
</div>

Such code is hard to follow and it's easy to get lost. Solution to this would be scoping value to tag it's invoked on and its children. Similar attribute is in Thymeleaf (http://www.thymeleaf.org/doc/tutorials/2.1/usingthymeleaf.html#local-variables). With name inspired by Thymeleaf, it would like this:

<div>
  ...
  <div data-sly-with.street="${person.details.address.street}">
    <span>${street}</span>
  </div>
  <!--span>${street}</span--> <!-- does not compile -->
  ...
</div>

What do you think?

paul-bjorkstrand commented 7 years ago

I do agree when seeing

<meta data-sly-use.headlibs="headlibs.js">

for the first time, I was confused as to why it wasn't in a wrapping div (or other element). At the same time, I can see a logic as to why they are script-scoped variables. I see some Javascript influence on the markup language, with respect to hoisting.

With your first example, it is still possible to do that today, with plain old data-sly-use, or the data-sly-test hack. I don't think set/alias with global status would harm any more than what is a fundamental part of the current model.

Although I think there may be some use for locally scoped variables, it would be a significant deviation (in my opinion) from the current model. I'm not saying that it is a bad deviation, per se, just stating an observation.

Thinking in line with your suggestion, I see some means to take aspects from the current spec and mold them into something like we want.

We already have data-sly-template and data-sly-call, perhaps we could use data-sly-with to mean "template and call immediately". Borrowing a lot of syntax from template & call here:

<div>
  ...
  <div data-sly-with="${ @ street=person.details.address.street }">
    <span>${street}</span>
  </div>
  <span>${street}</span> <!-- prints empty span, as if street never existed, the same as if d-s-template and d-s-call were use -->
  ...
</div>

There is a down side to my idea, as templates do not inherit their callers' scopes. you would literally get a new sightly scope in the block, having only access to the named and passed in variable(s), in addition to the standard globals. That could also be seen as a positive, though.

To me, this seems similar enough to the current spec, to be simple, yet expressive, for the problem we wish to solve. There would be a bit more of the semantics to go over, should Adobe take it up, but I personally like the way we are progressing with the idea(s).

karollewandowski commented 7 years ago

I don't think it's a significant deviation from the current model. Attributes data-sly-use and data-sly-test are hoisted, but there are data-sly-repeat/data-sly-list blocks which are scoped to the element (and children) they are applied to:

<ul data-sly-list.subPage="${currentPage.listChildren}">
    <li>${subPage.title}</li>
</ul>
${subPage.title} <!-- error -->

The data-sly-with could look and work the same (except the iterating of course ;) ).

deeprim commented 6 years ago

Hello @raducotescu, could you describe what the final solution would be please?

(need to know those to add proper support in AEM Tools plugin)

Thanks

raducotescu commented 6 years ago

@deeprim, the issue on is on the radar for 1.4. Once we formalise the spec we'll push a commit to the https://github.com/Adobe-Marketing-Cloud/htl-spec/tree/1.4 branch, before merging to master.

raducotescu commented 6 years ago

I've just pushed an update in 517911c. Essentially, data-sly-set will behave similar to data-sly-test, except for the evaluation of the provided expression.

Corroborated with #42 it should provide the feature requested in this issue.

Please let us know if the chosen solution doesn't match what was expected.

karollewandowski commented 6 years ago

To be honest it's better than nothing, but in my opinion global scope is much more error prone than scoped to element it would be plugged into. It can be wrapped by some element dependent on data-sly-test and depending on test evaluation result it would exist or not.

raducotescu commented 6 years ago

We did consider an element scope, but in the end we wanted the block element to be similar to data-sly-test and data-sly-use.