Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Next major version #17

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

Next major version #17

wants to merge 38 commits into from

Conversation

line-o
Copy link
Member

@line-o line-o commented Nov 14, 2022

The feedback from the community

NOTE: #22 is out of scope as it can be implement later as a configuration option

Embrace the top-down approach by allowing dynamic includes

with main page template, which is passed to templates:render

<html>
    <head>
        <title>${title}</title>
        <script data-template="templates:include"
            data-template-path="${includes?scripts}"></script>
        <style data-template="templates:include"
            data-template-path="${includes?styles}"></style>
    </head>
    <body>
        <!-- include template from module -->
        <nav data-template="templates:include"
            data-template-path="${includes?menubar}"></nav>
        <main data-template="templates:include"
            data-template-path="${includes?main}"></main>
    </body>
</html>

and a model, (the second parameter to templates:render)

map {
  "title": "My Page",
  "data": ((: this can be data loaded from the db for example :)),
  "includes": map {
    "scripts": "templates/includes/scripts.html",
    "styles": "templates/includes/styles.html",
    "main": "templates/pages/my-page.html",
    "menubar": "templates/includes/menubar.html"
  },
  "scripts": ( (: list of scripts to include in the page :) ),
  "styles": ( (: list of stylesheets to include in the page :) )
}

It is feasible to create quite flexible template structures that are easier to reason about. You do not need many, maybe no custom template function at all.

Don't worry templates:surround is still... around.

BREAKING CHANGES

  1. The first parameter to templates:apply has to be a single node now. It was allowed to pass in more than one node but this was untested and the output would have been invalid XML anyway. So this "feature" was dropped.
  2. The start and end delimiters for templates:parse-params (could be renamed to templates:substitute) must be set as configuration options now. This also means there is no way to change them somewhere in between.
  3. The option to filter out template attributes from the output is now on by default.
    In order to include the in the output you will have to set $templates:CONFIG_FILTER_ATTRIBUTES : false() in the options.
  4. The lib module was removed. Most of its functionality is now merged into the functions in the templates module. There is one exception: lib:resolve-apps is now part of a new module called tmpl-apps and the function was renamed to tmpl-apps:by-abbrev. It will put all resolved application paths into a map in the model under the apps key. Retrieval is done by <p id="test1">${apps?templating-test}</p>.

FEATURES

Function resolution

You can pass in xs:QName(?) as CONFIG_QNAME_RESOLVER instead of passing in a function that calls fn:function-lookup CONFIG_FN_RESOLVER. Due to a an issue in eXist-db's XQuery implementation (see eXist-db/exist#4614 ) this can only resolve functions from imported modules at the time of writing. The old approach can be used as before.
If you pass in none of the above the default resolver will still allow you to use all basic template functions in templates namespace.

Parameter resolution

The configuration option for parameter resolution can now contain a sequence of functions (function(xs:string) as item()*)* where it had to be a single function before. This allows for greater control over where parameter values are read from.
Defaults to

(
    request:get-parameter(?, ()),
    session:get-attribute#1,
    request:get-attribute#1
) 

Error reporting

when a template function cannot be resolved then the node is serialised in the error message to make it easier to find where the issue has occurred

New entry point

templates:render is a shorthand to templates:apply where the lookup is passed as part of the config

templates:render(
    <html><body></body></html>,
    map{},
    map {
        $templates:CONFIG_QNAME_RESOLVER : xs:QName(?),
        $templates:CONFIG_PARAM_RESOLVER : request:get-parameter(?,()),
        $templates:CONFIG_STOP_ON_ERROR : true(),
        $templates:CONFIG_FILTER_ATTRIBUTES : true()
    })

Max arity

With $templates:CONFIG_MAX_ARITY you can now specify the maximum arity of template functions. Defaults to 8 was 20.

Placeholder replacement

Before you had to declare data-template="templates:parse-params" (that was data-template="lib:parse-params" respectively in 1.1.0) to have placeholders in attributes and text nodes replaced.
This is no longer necessary and placeholders will always be replaced using the provided resolvers or the model.

Changes

  • value retrieval => new public function templates:resolve-key($model, $key) will call the configured parameter resolvers internally
  • lots of refactoring for readability, maintainability and performance

As the tests show this version does raise the minimum required processor version to eXist-db v5.4.1 because of NPEs in v5.2.0.

@line-o line-o marked this pull request as draft November 14, 2022 12:10
@line-o line-o added enhancement New feature or request help wanted Extra attention is needed labels Nov 14, 2022
@line-o line-o added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Nov 14, 2022
@duncdrum
Copy link
Contributor

@line-o please rebase

@line-o
Copy link
Member Author

line-o commented Nov 16, 2022

@duncdrum done! Thanks for merging the other PRs

@joewiz
Copy link
Member

joewiz commented Nov 21, 2022

@line-o p.s. As long as this involves breaking changes, I'm not sure if you want to resurrect a previously reverted breaking change from @wolfgangmm involving the lib:parse-params() function? See the details here: #8.

@line-o
Copy link
Member Author

line-o commented Nov 21, 2022

You are right @joewiz !
We should definitely include those in the next major version.

@line-o
Copy link
Member Author

line-o commented Nov 21, 2022

Also food for discussion is where to put which functions. As it is now possible to use only functions in the templates namespace, which functionality does it have to provide to be useful on its own?

  1. rendering a template (templates:render, templates:apply)
  2. loops (templates:for-each)
  3. subtemplates / partials / blocks (templates:include)
    • this was moved to lib but I would like to reverse that or make lib dependency of templates
    • include path should be possible from values in the model
      <a data-template="templates:include" data-template-path="model(page-template)" />
  4. output of values
    • some directive to allow lookup in the model
      <a data-template="templates:text" data-template-text="model(link-text)" />
  5. lookup values in the model (see above)
    • some way to tell the parameter resolution that this is a reference to a key in the model and has to be replaced with its value

@line-o line-o marked this pull request as ready for review December 8, 2022 13:43
@line-o line-o changed the title [WIP] Next major version Next major version Dec 8, 2022
@line-o
Copy link
Member Author

line-o commented Dec 8, 2022

@joewiz There is no need to reincorporate the reverted changes from #8 as the delimiters can no longer be set in templates but are now set once in the configuration.

$templates:CONFIG_START_DELIMITER: '\[\[',
$templates:CONFIG_END_DELIMITER: '\]\]'

If, however, this is not acceptable then we would have to look into this once more.

@yamahito
Copy link

yamahito commented Dec 9, 2022

Hello, sorry to be a bit late to the party!

I had some suggestions which I've previously raised with @joewiz for improving some aspects of the templating system; I've added them here as feature request #20 : is there any mileage in considering a similar solution as part of this next version?

Looking at the code, the implementation still looks fairly straightforward, with the suggested changes to templates:process now occurring in templates:process-element instead.

Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. We should check the documentation article for templating for necessary adjustments.

Needs another rebase

@wolfgangmm
Copy link
Member

I wonder if it would make sense to have parse-params enabled by default, which means it would be active unless you stop it for some part of the tree, e.g. with disable-parse. This would align with other templating engines (e.g. in Nunjucks you use raw to stop substitution).

@line-o
Copy link
Member Author

line-o commented Dec 13, 2022

I appreciate your feedback, honestly. Good stuff here @duncdrum @wolfgangmm

  • re Proposal for conditional processing improvements to eXist's HTML Templating Library #20 (@yamahito): It's definitely not too late. Conditionals is one aspect that should be nailed in the standard-library, but I have not fully understood the proposed change and its implications, yet.
  • re documentation (@duncdrum): A complete overhaul is likely due. Would you be open to help on that?
  • re parse-params (@wolfgangmm): I agree that parsing placeholders should be transparent to the user of the library. Can you imagine a case where you would not want placeholders to be replaced? Would you be open for a discussion on how to implement this? Use expand-blocks instead of process-children?

@duncdrum
Copy link
Contributor

@line-o i can help with the docs

@line-o
Copy link
Member Author

line-o commented Feb 5, 2023

@wolfgangmm your proposal is now part of this PR

@line-o
Copy link
Member Author

line-o commented Feb 6, 2023

I have an additional question regarding the handling of the now obsolete declaration of data-template="lib:parse-params".
Should templates that are rendered with this new version of templating that still include it throw or the declaration just be ignored?

  • if we throw than the legacy declarations will certainly be removed in templates
  • if we ignore those templates might be usable in 1.x.x and 2.x.x versions of templating (just the templates not the view module)

Is it worth it keeping this extra check in the library or even harmful as those declarations stay in there?
I am leaning towards throwing an error rather than ignoring the declaration.

@duncdrum
Copy link
Contributor

duncdrum commented Feb 6, 2023

+1 for throwing an error, explicit design >> black box design

line-o added 29 commits July 18, 2023 19:33
- refactor initial functions
  - templates:apply#3 and eXist-db#4
  - templates:render#3
- add function templates:configure called in all initial functions
- attribute filtering function is now selected by configuration instead
  main benefit is the if condition does not need to be called each time
- lib:include moved to templates:include
- lib:parse-params is now moved to templates:parse-params
- remove lib
- last remaining lib function lib:resolve-apps is now
  apps:by-abbrev
  links are now added to the model under key 'app'
  declare default value for links of apps that were not found
- delimiters are now part of the configuration and cannot be
  changed inside of templates
- add example (and test) for reading template-path from model
- refactor for speed and readability
- the default handling of duplicates when merging maps will change to
  use-first. Therefore set duplicate
  handling explicitly to use-last where it is crucial
This is the first version that implements map:merge#2
BREAKING CHANGES:
- $templates:START_DELIMITER => $templates:CONFIG_START_DELIMITER
- $templates:END_DELIMITER => $templates:CONFIG_END_DELIMITER

- All errors are prefixed with E_
- All settings are prefixed with CONFIG_
In an effort to declare all dependencies explicitly.
- add comment if surround template was not found
- avoid *-selector in XPath expression
Simplify check for problematic options.
The $selected variable either contains the attribute setting an
option as selected or an empty sequence.
This is in order to drop them from being the default in the next step.
- apps is now tmpl-apps to make it obvious it is not an application module
- tmpl-util contains utility functions that clobber up the core library
  (unused for the moment)
- build regular expressions for parse-params at configuration and re-use
  those throughout the code base
- remove functions from template that are now in tmpl-util
- use tmpl-util:* replacements in templates functions
This deprecates templates:parse-params as placeholders in
text and attribute nodes will always be replaced.

Before:  `<span data-template="templates:parse-params">[[item]]</span>`

Now : `<span>[[item]]</span>`

Any data-template attribute with the value "templates:parse-params" is
ignored.
Always throw an error if a declared template function cannot be found.
The attempt to ignore certain function names
creates more problems than it solves as it does not
incentivise to clean up templates when switching to
the newer templating library.
The test for templates with a legcacy declaration was apapted.
eXist-db will now return status code 500 server error when an exeption
is thrown during query execution (see
eXist-db/exist#4639).

Since the templating library is not concerned with routing
we simply add this other accepted status code to the list.
Tests expecting errors will pass with both satus code 400 and 500.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants