Refactor Navigation Include to use ESI

[trieloff]

See https://github.com/adobe/helix-helpx/pull/3/files#r194838365 – note that this depends on adobe/project-helix#171

[kptdobe]

The refactoring must consist into:

• remove the js code that "collect the nav": https://github.com/adobe/helix-helpx/blob/master/src/default.pre.js#L44-L63
• refactor the htl template to use the ESI command to include /summary.html (line https://github.com/adobe/helix-helpx/blob/master/src/default.htl#L15)

Up-coming questions / issues:

• /summary.html will be rendered with the default template... infinite loop!!
• we'll then need a nav.htl template that renders only the required HTML (would also get rid of the commiters data and other useless stuff). summary.md would then need to be changed to define the template! Or template needs to be solved differently!

Pre-requisites:

• ESI support in Petridish - https://github.com/adobe/project-helix/issues/171

[davidnuescheler]

actually comparing the original solution with the rendering directly in the default.htl and the separate nav.htl solution, i am not sure I like the ESI include better.
it seems that it introduces quite a bit of complexity for little apparent reason. I would generally agree that if the nav component was reference from a number of places this could be useful, but in this specific case, looking at the amount of source code added, it seems to just complicate things.

[kptdobe]

I disagree.
Having a clean and clear split makes the code much readable and the structure of the content more transparent. In the previous solution, all the nav handling was done in the default.pre.js including the "fetch" of the SUMMARY.md file.

A third option could be to use a cq:include-like htl command to include the nav.htl inside the default.htl which I do not like neither because still means that some code in the default.pre.js must "fetch" of the SUMMARY.md file and enhance the resource with the parsed md.

While I did not like the use of another "technology" at the beginning (introduce a new esi tag in the htl template), I now like the fact that this makes transparent the fact that the nav is managed in an other markdown (fragment ?).
Imagine now the nav requires a little of rendering logic (a more complex htl code), same for the header, footer... this will all end up in the default.htl template which will become huge while the esi model allows to delegate.

[davidnuescheler]

i totally agree that separating concerns into the seperate resources is cleaner.
however, just looking at the lines of code, the additional concept of ESI that i need to understand, just makes things more complicated. the duplication of much of the .pre.js code doesn't help, and i appreciate that we could (and should) hide that.

your statement that the rendering code could be more complicated is absolutely correct, but it is not in this very example.

as for ESI, in the case of the navigation that contains the same markup on all pages, one could argue that ESI is not the right solution either but instead it would be a separate resource (json, js, html, md?) that is requested directly from the client and therefore cached in the browser.

of course i agree that there is a place for ESI (and client side includes and htl-includes), i just think that in this very example, it is a lot easier for someone who is new to the project to just have to understand how default.htl and the default.pre.js work and be done.

i think we have a tendency to over-engineer things because we are anticipating additional complexity and extension needs or we are striving for abstraction and with that make the initial learning curve steeper.

[davidnuescheler]

@kptdobe, as i was reading #8, i just noticed that we possibly may have different goals for this example project.

i think you are looking at this as a project to try out a variety of solutions for correctly anticipated real-life problems, and i am looking at it in a way where i think how can we keep it as simple as possible to show how little complexity you will need to master to get started.

i think both goals are fine, and we should probably do both. maybe separately.

[kptdobe]

Correct and I sensed that already ;)

But my main goal is more to be... convinced and convincing.
With AEM with started the same way (roughly "dump the whole code in the JSPs") and it was simple and easy at the beginning but in the end we never really convinced the developer eco-system (messy code organisation, maven, clientlibs, to much to learn...). Simplicity is key I agree but not to the price of code maintenance, testing, performance...

So I agree to have "master" branch for simplicity and other branch(es) for real-life approach

Regarding ESI, I can revert the changes from master and move them to the "other" branch for now. I do not say ESI is THE solution but the previous code is pretty ugly (during the rendering of the md, it fetches the nav md, executes the lambda function to render it, computes some metadata you do not need... not so simple and smart).

[kptdobe]

As agreed, I reverted the changes: the "nav" is generated via code in the default.pre.js file.

See commit: dbbef1e

[kptdobe]

Still WIP: moving code to new "experiments" branch

[davidnuescheler]

(roughly "dump the whole code in the JSPs")

i think things are slightly different here than they were, in two ways:

1. sling and all it's predecessors always came with per resource script resolution that always split up your code into dozens of small files almost by default.

2. the separation of concerns that you are referring to in my books in mainly related to mixing display logic with business logic. i would argue that this is supported in a clean way through the .pre.js

i completely agree with you on the question of the code being maintainable, and i don't know a more real-world project would yield a different code organization, but i would like to make sure that we keep simple projects simple and provide the necessary means, for a bigger more complex project to modularize things as needed.