TryGhost / express-hbs

Express handlebars template engine with inheritance, partials, i18n and async helpers.
MIT License
458 stars 76 forks source link

Fixed inline async helpers not working in isolation #251

Closed allouis closed 1 year ago

allouis commented 1 year ago

refs https://github.com/TryGhost/express-hbs/commit/b8adbc380a4f02f8c459d15507777eb88364486d refs https://github.com/TryGhost/express-hbs/commit/2aaf916324cd6b30849d7514a498ab2bf5ed873d refs https://github.com/TryGhost/express-hbs/commit/63b62960bcd798ca15ba94f0ed43866182c975ca

This one took a little while to fully understand and fix.

TL;DR We have a concept of an "async id" which is used as a placeholder in the content whilst we wait for the async helpers to resolve. We were not correctly searching the document for these ids which resulted in incorrect behaviour.

The fix is to search only for the id prefix which does not have escapable characters, ensuring that we find all possible async ids.

  1. In the beginning express-hbs did not support nested async helpers. This meant that we did not need to constantly check the document for unresolved helpers, instead we could wait until all async helpers had resolved, and then replace all of the values in the document.

    • At this point we used __base64__ as the async id
  2. Then we supported nesting of async helpers with in a "contentFor" block. In order to optimise when we replaced async values in one of these blocks, we wanted to be able to search the text for usage. So we prefixed the temporary async place holder with an unlikely string.

    • At this point we used __aSyNcId__base64__ as the async id
    • See commit number 1 above
  3. Then we wanted to fix the correct escaping of async helpers when used "inline" (e.g. {{asyncHelper "foo"}}). To do this we introduced a new character to the async id, one that would be escaped by Handlebars.

    This then allowed us to know whether the resolved async value should be escaped, based on whether or not the async id it belonged to was escaped in the document!

    • At this point we used __aSyNcId_<_base64__ as the id.
    • See commit number 2 above
  4. Then we wanted to fix nested async helpers everywhere, and we refactored how they worked. The support for "contentFor" blocks was picked up with this as well. Essentially what we would do is render the output, check if there was at least one async id inside & replace all of them, this would recurse until no more async ids were found.

    However! Part of this refactor meant that we searched the document for async ids including the escapable character. So if there was only one helper used, and the output was escaped, the recursion would end, and we would have incorrectly output once of these async ids.

    • See commit number 3 above
allouis commented 1 year ago

@naz Oh nice I didn't find that!!