search

adobe / helix-helpx

Templates and Components for HelpX
Apache License 2.0
3 stars 4 forks source link

Refactor Navigation Include to use ESI #4

Open trieloff opened 6 years ago

trieloff commented 6 years ago

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

kptdobe commented 6 years ago

The refactoring must consist into:

Up-coming questions / issues:

Pre-requisites:

davidnuescheler commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

See commit: https://github.com/adobe/helix-helpx/commit/dbbef1ee2e2d2751a601dbace5a1de351309d16e

kptdobe commented 6 years ago

Still WIP: moving code to new "experiments" branch

davidnuescheler commented 6 years ago

(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.