Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Twitter's Improvements to Mustache.js #70

Closed
wants to merge 26 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

bcherry commented Dec 8, 2010

Hello!

As you know, we use Mustache.js on the new Twitter.com. We've had to make a few changes to support our use, which I've just finished getting officially integrated into our fork on GitHub, with tests in the Mustache.js suite. We'd love to be able to port these changes upstream to the wider community.

The main changes we've made are:

  1. Added i18n support, through {{_i}}{{/i}} tags. The README in our fork has more info on how this works.

  2. Fixed a bug we uncovered recently, where the content of section tags would get rendered twice, allowing Mustache injection. We fixed this silently since it was a pretty embarrassing bug in the site, the fix just hit our production site today. I'd be happy to provide more details on this, but we've also added a regression test to the suite in our fork.

  3. I've added Rhino as a test runner, in addition to running with jsc. We actually found some subtle regex-related bugs that caused failures in Mustache in Rhino and v8 only, so more coverage here seemed warranted. In the longer run, it would be nice to be able to also run the suite on a web page with QUnit or something, so you can try it in all browsers you'd like.

For now, we've changed the version to "0.3.1-twitter" to make it clear in our codebase, but we'd love to integrate with the main branch.

Thanks!

Contributor

bcherry commented Dec 8, 2010

oops, I meant to make the pull request from our "master" (it's exactly the same, I just merged this development branch in there).

Thanks for giving back :)

Do you think i18n could be implemented using higher order sections, rather than by adding a new tag? You could even name your lambda _i and you wouldn't have to change your templates :)

Contributor

bcherry commented Dec 9, 2010

Yeah, we've actually thought about doing that anyways. The biggest concern we have is that we'd like i18n available to all template render calls, regardless of what's passed in. I think we can add a feature to register members of a global context that's always present with the Mustache engine, and register i18n there using higher-order sections. Does that sound reasonable?

I think there's a typo in the documentation with a misplaced underscore -- it should be passing {mode:"tweet_button"} instead of {_mode:"tweet_button"}

I re-implemented this in a branch at:https://github.com/schuyler1d/mustache.js/tree/compiler_twitter
It seems to pass all tests, disregarding white-space. The approach there is to do the gettext _() call on compile, so there's no extra time translating when rendering the template (after the first time).

Contributor

bcherry commented Dec 10, 2010

Yeah, typo. I've fixed that.

Have you made a request to merge the pre-compiler back to the main source tree? That would be a great improvement. For now I think we're still going to with higher-order sections for i18n, but I'd love to get this done in a precompilation step in the future.

Have you made a request to merge the pre-compiler back to the main source tree?
yes, here: #69

Hi. Just lurking, found this pull request, and thought it is a great feature for mustache.js. Is there any progress on this? Thank you!

Contributor

bcherry commented Dec 22, 2010

Yes, sorry I've gotten occupied with other things. I expect to work on this more over the Christmas holiday.

I ran into the issue you fixed in your don't double-render commit today. I'd really like to see it merged.

I think with the global _(), and many of us mustachioed folks using underscore.js, the internationalization improvements are unlikely to be merged in as they are right now. One small change that would make it more palatable would be to pass the gettext function to Mustache.to_html(). Perhaps it would be better if pragmas were extended with (more) hooks and the ability to take options. So the signature would be Mustache.to_html(template, view, partials, options).

Contributor

bcherry commented Feb 21, 2011

So I'm going to split out the bug fix and test improvements, and submit a new pull request. I'll keep our i18n stuff in a branch, and then continue working on that, submitting a pull request when it's ready. Thanks for all the suggestions, everyone!

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment