documentcloud / jammit

Industrial Strength Asset Packaging for Rails
http://documentcloud.github.com/jammit/
MIT License
1.16k stars 197 forks source link

JST revisited #43

Closed agibralter closed 14 years ago

agibralter commented 14 years ago

Hi there -- I'm back for more trouble :).

Recently I realized that soon I'm going to need better JST functionality than 0.4.4 has (putting JST in with the JS, etc.), so I decided to take another stab at it. I know you want to wait for Rails 3 for 0.5.x, but I think this JST stuff is worthwhile enough to release as a 0.5.0 and then have 0.6.0 be Rails 3 (since the JST changes introduce deprecations, it shouldn't be 0.4.5).

In my dev branch (http://github.com/agibralter/jammit/commits/dev) I just pushed my latest commit:

http://github.com/agibralter/jammit/commit/2be5d1ae76eea6131d147aa2b75bec170c8b7b3a

The commit message say most of it, but basically, I rearranged test fixture names because they were becoming a little bit hard to maintain, I added a controller test finally :), and I made it so that people can specify template extensions with a config option (e.g. template_extension: "html.mustache"). I also made a pretty important change to how dynamically generated JS templates are handled (see the __templates__ stuff in the commit). I think this change makes Jammit's development behavior a lot more understandable.

I think there's one more piece of functionality that I want to add: proper scoping for JS templates with a template_base_dir config option. The way I see this working is that if a user specifies this config option, Jammit will build a nested object for all JST files located in this base directory.

For assets.yml:

template_base_dir: app/mustache_templates
template_extension: "html.mustache"
javascripts:
  default:
    - app/javascripts/*.js
    - app/mustache_templates/**/*.html.mustache

And a dir structure of:

You would get a jst part:

(function(){
window.JST = window.JST || {};
var template = function(str){var fn = new Function('obj', 'var p=[],print=function(){p.push.apply(p,arguments);};with(obj){p.push(\''+str.replace(/[\r\t\n]/g, " ").replace(/'(?=[^%]*%>)/g,"\t").split("'").join("\\'").split("\t").join("'").replace(/<%=(.+?)%>/g,"',$1,'").split("<%").join("');").split("%>").join("p.push('")+"');}return p.join('');"); return fn;};
window.JST['home'] = template(...);
window.JST['users'] = {};
window.JST['users']['index'] = template(...);
window.JST['users']['show'] = template(...);
window.JST['posts'] = {};
window.JST['posts']['index'] = template(...);
window.JST['posts']['show'] = template(...);
})();

The problem I see here is what happens if you also have a app/mustache_templates/users.html.mustache file? Perhaps it could be:

(function(){
...
window.JST['home'] = template(...);
window.JST['users/index'] = template(...);
window.JST['users/show'] = template(...);
window.JST['posts/index'] = template(...);
window.JST['posts/show'] = template(...);
})();
agibralter commented 14 years ago

Alright so I added a template_base_path config option in my latest commit on the dev branch:

http://github.com/agibralter/jammit/commit/735c78cf459054287e1eeb0f197bb813270ca4d9

I went with the second approach: window.JST['users/index'] = template(...);

Thoughts?

jashkenas commented 14 years ago

I think that both having all of this in a single commit -- and some of the changes that are in here, add a good deal more complexity than is necessary for the templates. If you feel like splitting this up into chunks, the parts I'd love to merge are:

agibralter commented 14 years ago

Cool, I'll try to get around to it asap.

As for the extensions -- I agree. Would it make sense to specify both a function and a namespace for each type of JST extension that you want recognized?

templates:
  - function:  "Mustache.createTemplate"
    namespace: "Templates.mustache"
    extension: "html.mustache"
    base_path: "app/templates"
  - function:  "JST.createTemplate"
    namespace: "Templates.jst"
    extension: "jst"

Complexity... I assume you're mainly referring to the template_base_path option. I've found this functionality really indispensable for having any sort of robust templating system. As jammit currently handles JST, it shoves every template into the same top-level namespace. This means that all your templates have to have long and complicated names and that there is no advantage to organizing the templates into directories as you would expect to do when you have a ton of templates. Furthermore, I've switched to mustache and I've been sharing my templates between Rails and Javascript. Basically what I do is I render the template on page load with mustache.rb and then update the page with ajax requests for JSON objects that update the templates with mustache.js. Mustache.rb expects a sensible app/templates directory. Any thoughts on that front? I'm using this setup in development/staging/qa and moving to production soon -- so far, it's working beautifully.

jashkenas commented 14 years ago

Sounds like a lovely setup.

In a perfect world, there would already be canonical-ish extensions for each of the template function types we'd like to support, and we could do the mapping without having to ask the developer to configure it. But in this world, perhaps it would be best to leave things as they are, and simply add your suggested template_extension option. Sorry to have swung around full-circle on that one.

As to the template_base_path bit ... why don't we just compute that? We can figure out the common prefix from the list of template paths, and knock that portion off. No need to name it.

agibralter commented 14 years ago

No worries! :)

Definitely could compute the template_base_path -- I was just trying to maintain backwards compatibility: for example, if you're using Jammit with templates right now and you have your templates arranged in directories for some reason, your JS code should still expect the templates to be named by filename only. Introducing this automatic base_path computation might break people's JS. How about some sort of auto option: template_base_path: true would compute based on common prefixes; template_base_path: false would be the default (for legacy) and would exhibit the current filename-only behavior; finally, template_base_path: some/path would allow the user to specify some other base path.

jashkenas commented 14 years ago

Well, since 0.5.0 is already backwards-incompatible with respect to templates ... if you sneak this patch in the door, I think it would be fine to go along with.

agibralter commented 14 years ago

:) -- timing on the 0.5.0 release?

jashkenas commented 14 years ago

I'm just waiting for someone to confirm that it's working with Rails 3... And this, if you want to go ahead with it.

agibralter commented 14 years ago

Alright -- I'll take a stab this evening/tomorrow.

agibralter commented 14 years ago

Ok just putting this all in one commit now and working on some tests -- just wanted to get your opinion on something: how do you feel about the dynamic path used for template files when package_assets: off (dev mode) is set? In my patch I append __templates__ to the name of the JS package so that the Jammit controller can know when to generate a dynamic template package. I think the alternative is to have the URL for these dev mode dynamic template packages be "#{js_package_name}.#{template_extension}". I can't remember why I picked __templates__... do you follow what I mean here?

jashkenas commented 14 years ago

yep... package_name.template_extension sounds like a fine URL for dev mode to me.

agibralter commented 14 years ago

Oh yeah... now I remember why I picked __templates__... it's because I didn't want to mess with Jammit::Controller::VALID_FORMATS and Jammit::Controller#parse_request. If we go with the "#{js_package_name}.#{template_extension}" dynamic template path, we'll have to have the Jammit controller accept requests for JST formats like ".jst" and ".html.mustache"...

agibralter commented 14 years ago

Oh and I misread your earlier message -- I thought you wanted everything in one commit. Hmm I'm not sure what to do with all the test-file renaming. I know it's a bit of overkill, but having the test fixtures rearranged really helps with the testing... I think this whole thing is pretty manageable as one cleaned up commit. Why don't I push it on a branch and you can see if the tests run for you?

jashkenas commented 14 years ago

Accepting ".html.mustache" in development seems like the right thing to do, even if it's a little more work on our (your) end. I don't mind about having one commit on a branch -- sounds like a good plan.

agibralter commented 14 years ago

Cool -- will fix that up after lunch.

agibralter commented 14 years ago

Alright this branch contains a more concise patch: http://github.com/agibralter/jammit/tree/jstweaks

It's 2 commits right now:

I think the last thing missing is having the controller test run with Rails 3... Any thoughts on that front? When I require 'rails' in the controller test with Rails 3 it blows up the whole test suite.

jashkenas commented 14 years ago

This is merged to master. No thoughts on the Rails 3 issue, but let's ask the Rails 3 folks in the other thread.

agibralter commented 14 years ago

Cool, thanks.