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 to cache by tags in addition to template string #643

Merged
merged 2 commits into from
Aug 26, 2017
Merged

Writer.prototype.parse to cache by tags in addition to template string #643

merged 2 commits into from
Aug 26, 2017

Conversation

raymond-lam
Copy link
Contributor

Resolve #617
Because the results of Writer.prototype.parse depends on both the template string and the tags, it should cache by both

Copy link
Collaborator

@dasilvacontin dasilvacontin left a comment

Choose a reason for hiding this comment

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

LGTM, good catch!

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, ['(', ')']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are parenthesis, not braces, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my bad. I'll fix it.

@raymond-lam
Copy link
Contributor Author

@dasilvacontin Addressed your comment -- good catch! I actually meant to test the template with Mustache.tags as the delimiters, with it being braces.

Copy link
Collaborator

@dasilvacontin dasilvacontin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix! :)

@dasilvacontin dasilvacontin merged commit b283da5 into janl:master Aug 26, 2017
@raymond-lam
Copy link
Contributor Author

@dasilvacontin Awesome, thanks!

@dasilvacontin
Copy link
Collaborator

I started refreshing my memory about the other PR, but I don't have enough time today. Will do tomorrow morning.

@raymond-lam
Copy link
Contributor Author

@dasilvacontin no rush! Thanks!

@raymond-lam raymond-lam deleted the issue617 branch August 26, 2017 16:18
neocotic added a commit to tmplat-extension/tmplat-mustache that referenced this pull request Nov 14, 2017
This reverts commit b283da5, reversing
changes made to 23beb3a.
@petrkoutnysw
Copy link

I am afraid this PR caused breaking change for us.
We are doing following:

  1. parse our template with custom tags
  2. render the template - according to mustache.js:465 our custom tags are not used, hence the tokens are different and custom tags are not recognized

@phillipj
Copy link
Collaborator

phillipj commented Aug 8, 2018

@petrkoutnysw a bug fix landed after this got merged: #664. This is also what's included in v2.3.1 of mustache.js.

Are you sure you're using the latest version? Or have I misunderstood and you have found another bug?

@petrkoutnysw
Copy link

@phillipj Yeah I have 2.3.1.

It is quite easy - if you take a look at parse function (line 465), it creates a cache using tags if cache does not exist already. Then you call render function that calls parse internally without specifying tags. Thus it is getting wrong tokens. Hope it makes sense, I have not been reading mustache code previously.

Anyway, we froze our mustache to 2.3.0.

@raymond-lam
Copy link
Contributor Author

@petrkoutnysw Do you have replication steps (the template you used and how you made the call to render it)?

@raymond-lam
Copy link
Contributor Author

raymond-lam commented Aug 9, 2018

@petrkoutnysw

It tried the following code:

mustache.parse('<<foo>>bar', ['<<','>>']);
mustache.render('<<foo>>bar', { foo:'biz'})

Which outputs <<foo>>bar as expected.

Then, if I do:

mustache.tags=['<<','>>']
mustache.render('<<foo>>bar', { foo:'biz'});

It outputs bizbar again as expected.

How are you setting the custom tags (there's only one way as far as I can tell) and calling the render function?

render does indeed call parse without passing in a tags argument, but the cacheKey in parse is defined to fallback to mustache.tags. (As far as I can tell, the tags argument is never used internally by mustache.js.)

445   Writer.prototype.parse = function parse (template, tags) {
446     var cache = this.cache;
447     var cacheKey = template + ':' + (tags || mustache.tags).join(':');
448     var tokens = cache[cacheKey];
449
450     if (tokens == null)
451       tokens = cache[cacheKey] = parseTemplate(template, tags);
452
453     return tokens;
454   };

@petrkoutnysw
Copy link

@raymond-lam Give me some time please, I am on conference and don't have time to reply right now. Thanks.

@phillipj
Copy link
Collaborator

Maybe these two gives valuable input here? #669 & #662

@raymond-lam
Copy link
Contributor Author

@phillipj @petrkoutnysw Ah yes I see those reported issues now -- I'll have a look.

phillipj pushed a commit that referenced this pull request 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
phillipj pushed a commit that referenced this pull request Aug 25, 2018
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

Successfully merging this pull request may close these issues.

4 participants