-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, good catch!
test/parse-test.js
Outdated
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, ['(', ')']); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@dasilvacontin Addressed your comment -- good catch! I actually meant to test the template with |
There was a problem hiding this 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 Awesome, thanks! |
I started refreshing my memory about the other PR, but I don't have enough time today. Will do tomorrow morning. |
@dasilvacontin no rush! Thanks! |
I am afraid this PR caused breaking change for us.
|
@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? |
@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. |
@petrkoutnysw Do you have replication steps (the template you used and how you made the call to render it)? |
It tried the following code:
Which outputs Then, if I do:
It outputs How are you setting the custom tags (there's only one way as far as I can tell) and calling the
|
@raymond-lam Give me some time please, I am on conference and don't have time to reply right now. Thanks. |
@phillipj @petrkoutnysw Ah yes I see those reported issues now -- I'll have a look. |
Resolve #617
Because the results of
Writer.prototype.parse
depends on both the template string and the tags, it should cache by both