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

Writer.prototype.parse caches by template string, ignoring tags #617

Closed
raymond-lam opened this issue Dec 27, 2016 · 1 comment
Closed

Comments

@raymond-lam
Copy link
Contributor

I haven't run into actual issues with this, but I noticed that Writer.prototype.parse caches the result, where the cache key is just the template text. Therefore, these tests would fail:

describe('Mustache.parse', function () {
...

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

  describe('when parsing a template with tags specified then a 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);
    });
  });

  describe('when parsing a template after already having parsed that template with a different Mustache.tags', function() {
    it('returns different tokens for the latter parse', function() {
      var template = "{{foo}}[bar]";
      var parsedWithBraces = Mustache.parse(template, ['(', ')']);

      var oldTags = Mustache.tags;
      Mustache.tags = ['[', ']'];
      var parsedWithBrackets = Mustache.parse(template);
      Mustache.tags = oldTags;

      assert.notDeepEqual(parsedWithBrackets, parsedWithBraces);
    });
  });
});

This issue is easily solved by changing Writer.prototype.parse to this (the change being to include the tags in the cache key):

  Writer.prototype.parse = function parse (template, tags) {
    var cache = this.cache;
    var tokens = cache[template];

    if (tokens == null)
      tokens = cache[template + ':' + (tags || mustache.tags).join(':')] = parseTemplate(template, tags);

    return tokens;
  };
  1. It's unlikely that one would actually run into this bug in real life, though parse would be more correct if this simple fix was include. Do we want to fix this bug, even if it is unlikely to manifest?
  2. One could argue that my suggested fix above breaks separation of concerns or some principle of abstraction. Writer.prototype.parse is simply a memoization of parseTemplate -- its only concern should be to cache the results of a parse. The actual details of how parsing is accomplished is in parseTemplate, and the fact that parsing falls back onto mustache.tags is one such detail. Referencing it in Writer.prototype.parse would be introducing a leak into the parseTemplate abstraction. Thoughts?
@raymond-lam
Copy link
Contributor Author

@dasilvacontin any thoughts on this?

phillipj pushed a commit that referenced this issue Aug 17, 2018
Pull requests #643 and #664 together fix the issue reported in issue #617, but the change in behavior is causing semvers concerns. See issue #669. Therefore, roll back #643 and #664 and later reintroduce in next planned major release (v3.0).

Fixes #669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant