The "tags" render parameter can now additionally accept a configuration object specifying custom tags and/or a custom escape function for that render case #764

pineapplemachine · 2020-10-09T08:19:25Z

This PR renames the tags parameter in the render function to config. The same previous behavior of passing a tags array is maintained, and in addition to this if a config object is passed then a custom escape function can be provided via its escape attribute in addition to a tags array via its tags attribute.

For example:

Mustache.render(template, view, partials, {
    tags: ["[[", "]]"],
    escape: escapeSomethingOtherThanHTML,
});

Tags can still be passed as an array argument and the escape function can still be modified globally. The above example is functionally equivalent to this code, which is still also fully supported:

const mustacheEscapeOriginal = Mustache.escape;
Mustache.escape = escapeSomethingOtherThanHTML;
Mustache.render(template, view, partials, ["[[", "]]"]});
Mustache.escape = mustacheEscapeOriginal;

The rationale for this addition is that providing an escape function here, when rendering a template, is a safer pattern for specifying an escape function than modifying the global mustache.escape attribute when a project may need to use mustache to render more than just one kind of content, e.g. both HTML and Latex templates in the same project.

This change is completely backwards compatible with the existing test suite. Documentation comments have been updated and thorough test coverage has been added for this new feature.

What this PR does not include: The separate TypeScript @types definitions package will need to also be updated to reflect that the previous tags parameter can now also optionally be a configuration object for the render function, as well as to reflect very similar additions or updates to the parameters of some functions that are called by the render, function where the configuration object needs to be passed through.

This PR incidentally includes two unrelated fixes that were required for me to build the package and run tests on my machine. (package.json build command now runs on Windows; eslint no longer fails tests/partial-test.js.)

Windows shell does not recognize 'single quotes' the same way as bash. The 'single quotes' in the build command have been changed to "double quotes" instead.

The linter enforces 'single quotes' but two strings in this test file were written using "double quotes". This issue was causing tests to fail.

If an object is provided for what used to be the `tags` argument instead of an array, then the object is treated as a "config" object. A config object can have a `tags` attribute which behaves the same as the `tags` argument, and it can also optionally have an `escape` argument that is used for providing a custom escape function for that render call. The rationale for this addition is that providing an escape function here, when rendering a template, is a safer pattern for specifying an escape function than modifying the global mustache.escape attribute when a project may need to use mustache to render more than just one kind of content, e.g. both HTML and Latex templates in the same project.

The `render` argument passed to a `function (text, render)` lambda function was not accounting for custom tags or other configuration. This issue has been fixed and a regression test has been added for this case.

pineapplemachine · 2020-10-09T08:40:18Z

mustache.js

    var self = this;
    var buffer = '';
    var value = context.lookup(token[1]);

    // This function is used to render an arbitrary template
    // in the current context by higher-order sections.
    function subRender (template) {
-      return self.render(template, context, partials);
+      return self.render(template, context, partials, config);


Bugfix: tags was not being passed through before. Now a tags array or a config object will be passed via the render function argument in a lambda section using the function (text, render) API.

pineapplemachine · 2020-10-09T08:40:49Z

package.json

@@ -28,7 +28,7 @@
    "url": "https://raw.github.com/janl/mustache.js/{version}/mustache.js"
  },
  "scripts": {
-    "build": "rollup mustache.mjs --file mustache.js --format umd --name Mustache --banner '// This file has been generated from mustache.mjs' && uglifyjs mustache.js > mustache.min.js",
+    "build": "rollup mustache.mjs --file mustache.js --format umd --name Mustache --banner \"// This file has been generated from mustache.mjs\" && uglifyjs mustache.js > mustache.min.js",


Bugfix: This change allows mustache.js to be built on Windows.

pineapplemachine · 2020-10-09T08:41:29Z

test/partial-test.js

@@ -159,7 +159,7 @@ describe('Partials spec', function () {
    });

    it('Nested partials should support custom delimiters.', function () {
-      var tags = ["[[", "]]"];
+      var tags = ['[[', ']]'];


Bugfix: This fixes an issue where eslint failed this file when attempting to run tests, since eslint enforces the use of single-quoted strings.

phillipj · 2020-10-09T12:43:23Z

Thanks a lot for the contribution and appreciate the elaborate description!

Haven't had the chance to look at the changeset yet, but from a distance, judging from the description, this sounds really good 👍

..will have to come back to you with a detailed review after having looked at the actual changes involved.

phillipj · 2020-10-10T11:13:58Z

mustache.js

@@ -644,7 +656,8 @@
      if (tagIndex == 0 && indentation) {
        indentedValue = this.indentPartial(value, indentation, lineHasNonSpace);
      }
-      return this.renderTokens(this.parse(indentedValue, tags), context, partials, indentedValue, tags);
+      var tokens = this.parse(indentedValue, tags);


Nice improvement to increase self descriptiveness 👍

phillipj · 2020-10-10T11:18:04Z

mustache.mjs

  var value = context.lookup(token[1]);
  if (value != null)
-    return typeof value === 'number' ? String(value) : mustache.escape(value);
+    return (typeof value === 'number' && escape === mustache.escape) ? String(value) : escape(value);


Reading && escape === mustache.escape gives me the impression we do not escape the value if the default escape function is being used?

..I certainly would have expected a test or two to explode badly if the default HTML escaping got removed by this change.

Since that's not the case, I guess I'm basically wondering why I'm not really grasping why this introduced change is needed 😄

This ternary condition was from a recent PR (#754) that was intended to optimize performance when writing numeric values. It works fine for escaping HTML, but someone might want their custom escape function to treat numbers differently from the default JS number stringification behavior. (Maybe they want thousands separators. Maybe they want to write hexadecimal values. Etc.) So now template rendering will continue using this optimized path when the escape function is the global default and skip it otherwise.

Perhaps one could make an argument for moving this optimization out of this function and into the default escape function? I'm fine making that change, I think it would certainly make the code more clearly legible if nothing else. It would also avoid a potential pitfall with modifying the global escape function and expecting to be able to write numbers differently, like one should now be able to do with an escape function provided via the config object.

Though I'd note that this pitfall is preserved behavior and not a newly introduced regression. Changing this behavior may break user code that was expecting their global custom escape function to never have to handle a number value.

Since there are other changes in this PR that may break code using custom tags or escape functions that is sensitive to some of these quirks (e.g. if there was any code that relied on the section rendering code discarding the tags argument when rendering its contents and going back to using the global default tags), I think the best thing would be to risk the breaking and move this check into the escapeHtml function. That function can just not go to the trouble of calling String.replace with a regex when the input is guaranteed to not contain HTML metacharacters, i.e. null, undefined, true, false, and numbers. Let me know what you think. If you agree, I'll go ahead and move that type test logic into escapeHtml instead.

Here is what a more invasive change to the value escaping functionality might look like.

All tests continue to pass without changes, so this is essentially backwards compatible, still. But it is likely to change behavior in ways that users relying on global escape function replacement might find inconvenient. If you like it I'll write some tests and make sure everything is solid. If not then the PR as it stands now still satisfies my own needs.

pineapplemachine/mustache.js@render-with-provided-escape-function...pineapplemachine:render-config-invasive

It works fine for escaping HTML, but someone might want their custom escape function to treat numbers differently from the default JS number stringification behavior.

Aaaah very good point indeed, makes perfect sense.

I did not think of that aspect when reviewing #754 to be honest. The fact that we've made it so that non-default escape functions won't be able to escape numbers as a result of that typeof value === 'number' check, is certainly not good. Thanks for pointing that out!

We'll have to reconsider that number check -- that can be done separate from this PR tho.

I fully agree with the rationale behind introducing && escape === mustache.escape that you shared above.

In danger of sounding too pedantic, I'm wondering if this change could be made in its own PR. Primarily to keep the history of changes as focused and "honest" as possible, without other seemingly unrelated changes introduced.

Also feel like what you've just brought up, is severe enough that it deserves the attention a dedicated PR & following discussions would have.

I'd like to make sure it's clear that the diff you're referring to will not change behavior when using a global escape function replacement. Whether && escape === mustache.escape is present or not affects the behavior of the newly introduced config object escape functions, and only config object escape functions. This particular diff is an effort to completely preserve previous behavior while allowing the useful new config object behavior to be more flexible than replacing the global escape function.

The abscence or presence of this check && escape === mustache.escape should only have a bearing on the newly added functionality that this PR is focused on. If you're thinking of the more invasive changes suggested in pineapplemachine/mustache.js@render-with-provided-escape-function...pineapplemachine:render-config-invasive, then I think you're right that making those changes a separate PR open for a separate discussion would be a good idea. If you're saying that this current PR should be broken up, then I don't think I'm on the same page. Unless I made an unintentional mistake, the previous behavior should be completely preserved, with the exception that sections will no longer discard a tags argument when rendering their contents, since I thought that was pretty clearly unexpected behavior. (#764 (comment)) Besides that one exception related to how sections are rendered using custom tags, this PR is 100% a new added feature and 0% changing how global escape function replacement behaves.

This particular diff is an effort to completely preserve previous behavior while allowing the useful new config object behavior to be more flexible than replacing the global escape function.

Truly appreciated, impressive piece of work 👏

Thanks for your point of view on what would be a good fit for a followup discussion & PR targeted to what you've raised related to escaping number values.

phillipj · 2020-10-10T11:20:43Z

mustache.mjs

 };

 Writer.prototype.rawValue = function rawValue (token) {
  return token[1];
 };

+Writer.prototype.getConfigTags = function getConfigTags (config) {
+  if (Array.isArray(config)) {


Did you consider using our locally defined isArray() function like we do elsewhere?

Very good point. Fixed 👍

phillipj · 2020-10-10T11:34:36Z

Really nice work!

Aside from the questions raised, this LGTM.

phillipj

LGTM

phillipj · 2020-10-12T17:44:14Z

Let's keep it up for a few days extra before merging, just in case someone else jumps in with valuable input.

phillipj · 2020-10-22T19:07:55Z

Thanks a lot for this contribution @pineapplemachine!

Seeing these changes as additive (+ bugfixes) so we'll probably release it as mustache.js v4.1 shortly.

phillipj · 2020-12-06T00:06:59Z

Pushing v4.1.0 obviously took way too long, finally managed to push it to npm now.

phillipj · 2020-12-06T20:57:40Z

FYI @types/mustache v4.1.0 has also been published to allow use of this newly added functionality for TypeScript users.

pineapplemachine · 2020-12-07T16:21:21Z

Sounds great, and thank you for covering the TypeScript definitions 👍

pineapplemachine added 5 commits October 9, 2020 10:48

Modify build command so that mustache.js can be built on Windows

35322d1

Windows shell does not recognize 'single quotes' the same way as bash. The 'single quotes' in the build command have been changed to "double quotes" instead.

Fix eslint failure in test/partial-test.js

15f46b7

The linter enforces 'single quotes' but two strings in this test file were written using "double quotes". This issue was causing tests to fail.

Add test coverage for new render config object parameter

2d38c6d

Fix behavior when rendering lambda sections & add regression test

ab9be15

The `render` argument passed to a `function (text, render)` lambda function was not accounting for custom tags or other configuration. This issue has been fixed and a regression test has been added for this case.

pineapplemachine commented Oct 9, 2020

View reviewed changes

phillipj reviewed Oct 10, 2020

View reviewed changes

Use outstanding isArray function in new render config object code

6726b91

phillipj approved these changes Oct 12, 2020

View reviewed changes

phillipj merged commit 224fe3a into janl:master Oct 22, 2020

phillipj mentioned this pull request Dec 6, 2020

[mustache] Add .render() options parameter introduced in v4.1.0 DefinitelyTyped/DefinitelyTyped#49974

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The "tags" render parameter can now additionally accept a configuration object specifying custom tags and/or a custom escape function for that render case #764

The "tags" render parameter can now additionally accept a configuration object specifying custom tags and/or a custom escape function for that render case #764

pineapplemachine commented Oct 9, 2020 •

edited

Loading

pineapplemachine Oct 9, 2020

pineapplemachine Oct 9, 2020

pineapplemachine Oct 9, 2020

phillipj commented Oct 9, 2020

phillipj Oct 10, 2020

phillipj Oct 10, 2020

phillipj Oct 10, 2020

pineapplemachine Oct 10, 2020 •

edited

Loading

pineapplemachine Oct 10, 2020 •

edited

Loading

pineapplemachine Oct 10, 2020

phillipj Oct 11, 2020

phillipj Oct 11, 2020

pineapplemachine Oct 11, 2020 •

edited

Loading

phillipj Oct 12, 2020

phillipj Oct 10, 2020

pineapplemachine Oct 10, 2020

phillipj commented Oct 10, 2020

phillipj left a comment

phillipj commented Oct 12, 2020

phillipj commented Oct 22, 2020 •

edited

Loading

phillipj commented Dec 6, 2020

phillipj commented Dec 6, 2020

pineapplemachine commented Dec 7, 2020

The "tags" render parameter can now additionally accept a configuration object specifying custom tags and/or a custom escape function for that render case #764

The "tags" render parameter can now additionally accept a configuration object specifying custom tags and/or a custom escape function for that render case #764

Conversation

pineapplemachine commented Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipj commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pineapplemachine Oct 10, 2020 • edited Loading

Choose a reason for hiding this comment

pineapplemachine Oct 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pineapplemachine Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipj commented Oct 10, 2020

phillipj left a comment

Choose a reason for hiding this comment

phillipj commented Oct 12, 2020

phillipj commented Oct 22, 2020 • edited Loading

phillipj commented Dec 6, 2020

phillipj commented Dec 6, 2020

pineapplemachine commented Dec 7, 2020

pineapplemachine commented Oct 9, 2020 •

edited

Loading

pineapplemachine Oct 10, 2020 •

edited

Loading

pineapplemachine Oct 10, 2020 •

edited

Loading

pineapplemachine Oct 11, 2020 •

edited

Loading

phillipj commented Oct 22, 2020 •

edited

Loading