Custom delimiters on Mustache.parse() not working since 2.3.1

[mbrodala]

Since version 2.3.1 custom delimiters apparently don't work anymore for Mustache.parse(). See the following examples:

• 2.3.0, working, values are inserted: https://codepen.io/mbrodala/pen/vaQPxK
• 2.3.1: broken, values are not inserted: https://codepen.io/mbrodala/pen/NBEJjX

This is most likely related to #663 and its fix. Notice that I can restore this by using the newer Mustache.tags = [...] instead: https://codepen.io/mbrodala/pen/QBJoOx

Can you please have a look at this?

[phillipj]

Thanks a lot for the report @mbrodala, those codepens are much appreciated!

[raymond-lam]

@mbrodala Thanks for the codepens.
I wonder if there's been a misunderstanding here.

#643 and #664 fix a bug which I reported in #617 , which is illustrated by this test, which accompanies #643 :

  describe('when parsing a template with tags specified followed by the same template with different tags specified', function() {
     it('returns different tokens for the latter parse', function() {
       var template = "(foo)[bar]";
       var parsedWithParens = Mustache.parse(template, ['(', ')']);
       var parsedWithBrackets = Mustache.parse(template, ['[', ']']);
       assert.notDeepEqual(parsedWithBrackets, parsedWithParens);
     });
   });

The parse function was caching using only template as the cache key, so that the next time parse is used to parse that template, it would return exactly the same tokens, even if the specified tags are different.

tags is an optional parameter, and when it is omitted, it falls back to mustache.tags, which is by default ['{{', '}}']. The fall back mustache.tags is used as part of the cache key.

I think I know what's going on with regard to the bug fix and expectations, and I'll try to walk through it, and I'll use the codepen as the example.

v2.3.0

Mustache.parse(template, ['[[', ']]']);

In 2.3.0, this instructs Mustache to parse template, using ['[[', ']]'] as tags. Mustache does so and returns the correct result, but caches the call using only template. See lines 447-450 of [email protected]:

    if (tokens == null)
       tokens = cache[template] = parseTemplate(template, tags);

The next call in the codepen is:

var output = Mustache.render(
  template,
...

render does not take a tags parameter, so does not pass one to parse, so when render is called, parse uses mustache.tags as its tags. So, when that render call is made, it is effectively a telling parse, "Please parse template and implicitly use ['{{', '}}'] as tags." parse actually does the wrong thing and does a cache lookup completely ignoring both tags and mustache.tags. It happens to return the result of the template parsed with [['[', ']']], but only because the first call to parse in the entire program for that template was made with ['[[', ']']] as tags.

v2.3.1

Mustache.parse(template, ['[[', ']]']);

The parse result is cached using both template and tags, which is ['[[', ']]'] as the cacheKey.

The next call:

var output = Mustache.render(
  template,
...

render calls parse, passing template but omitting tags. parse therefore has tags fall back to mustache.tags, which remains the default ['{{', '}}']. parse does a cache lookup against the cache key of template and ['{{', '}}'], and incurs a cache-miss, as expected since parse had not yet been called with that combination of template and tags. It therefore parses template using ['{{', '}}'].

I believe v2.3.1 exhibits the correct behavior. If we were to change the codepen in https://codepen.io/mbrodala/pen/QBJoOx slightly and run it against v2.3.0:

var template = "[[item.title]] [[item.value]]";
Mustache.parse(template, ['[[', ']]']);
var output = Mustache.render(
  template,
  {
    item: {
      title: "TEST",
      value: 1
    }
  }
);
alert(output);

The output is [[item.title]] [[item.value]], which is not expected.

I can see how the behavior in https://codepen.io/mbrodala/pen/NBEJjX might be surprising, since the Mustache.parse and Mustache.render calls are right next to each other and one might not even realize that Mustache.parse even takes a tags argument. (Why does Mustache.parse even take a tags argument? It's never used anywhere in mustache.js -- parse simply defaults internally to mustache.tags...)

If the change in behavior really does defy the expectations of a bugfix release, then I'm not exactly sure what to do. One possibility is to release another bugfix version with #664 reverted, which in effect removes all caching behavior (given that in #643, all cache lookups will be misses). We could then put #664 back into the next major revision. Another possibility is to remove all caching in a bugfix release (as opposed to releasing a mustache.js with non-functional caching), and then put all the caching back in the next major revision. The former option probably has less risk (least amount of code change) but the latter option is probably more "correct". @phillipj thoughts?

[mbrodala]

Thanks a lot for the detailed research which fully makes sense from my POV.

I wouldn't mind about the change at all but given that it is impossible to pass tags to Mustache.render() to ensure a cache hit and that Mustache.parse() is advertised to cache the template (no mention of tags here) I wonder if this really should be reverted.

If we assume that one calls Mustache.parse with a custom set of tags we can also assume that the template uses these delimiters (BTW, "tags" vs "delimiters" should be cleared up too). Following that we can assume that a call to Mustache.render is expected to work, no matter how and if the given template is cached already and how it was compiled if that is the case. Right now this is not guaranteed in case custom tags are used.

[raymond-lam]

@mbrodala Yes that makes sense, though Mustache.parse(template, ['[[', ']]']); followed by Mustache.parse(template, ['((', '))']); giving exactly the same result would still be unexpected.

Here is a straw-man solution/compromise ("straw-man" because I don't like it but it's worth brainstorming). We could have parse cache against both template alone and template with tags. When parse is called with tags specified, then it does a lookup against template and tags. When we call render, which calls parse without tags, then we do a lookup against template alone. Thoughts?

[mbrodala]

Sounds off and basically is but will fix this issue while keeping the fix intact. OK from my POV.

[raymond-lam]

@mbrodala is the core issue that you can't pass tags into render? We could also just add a tags parameter to render.

[raymond-lam]

@petrkoutnysw Is this roughly the issue that you have been experiencing as well?

[mbrodala]

It's at least an inconsistency between parse() and render(). We wouldn't even use parse() if we could pass custom tags to render() indeed. And now with proper caching in place this becomes more obvious.

[ilkovich]

+1 for adding a tags parameter to render() to eliminate a lot of confusion -- we got bit by this change as well and the link b/w parse and render always seemed like a bit more magic than necessary.

[raymond-lam]

Okay, so how about we disable caching in a bug fix release, to fix the immediate issue and comply with semantic versioning, and reintroduce it and tags in the render method in the next major release? (Again, I’m not a huge fan of the straw-man solution I proposed.)

[phillipj]

Thanks a lot for that thorough walkthrough @raymond-lam!

I'm leaning towards the suggested bug fix release, mainly because of semver concerns and projects where this change of behaviour is unexpected and can therefore cause havoc in projects out in the wild.

Re-introducing the caching behaviour again in the planned next major release, we can include migration instructions in the release notes.

[raymond-lam]

@phillipj I've issued pull request #670 which rolls back #643 and #664. Rather than disabling caching all together, for the sake of risk mitigation, simply going back to v2.3.0 behavior (in a bugfix release) seems safest for dependents on Mustache v2.x.x. I will issue another pull request to reintroduce in a major release.

[raymond-lam]

@phillipj #671 reintroduces caching fixes, to wait on a major release.

[raymond-lam]

Created issue #672 to address adding tags to `render.

[mbrodala]

Thanks a lot for looking into and fixing this, you guys rock. 👍

[phillipj]

Hats off to @raymond-lam for this one! Thanks to you as well, crucial for us to know when unexpected changes happens out in the wild.

v2.3.2 has been published 🚀