documentcloud / jammit

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

template_base_path configuration option. #151

Closed dark-panda closed 13 years ago

dark-panda commented 13 years ago

The common path detection on templates produces an annoying side-effect where adding and removing templates to your Jammit configuration can cause the template names in the resulting JavaScript to change which may require code changes to point to the new template names. For instance:

javascripts:
  templates:
    app/views/something/a.jst
    app/views/something/b.jst

This would produce paths similar to JST['a'] and JST['b']. However, if you were to add another template in there with a different base path:

javascripts:
  templates:
    app/views/something/a.jst
    app/views/something/b.jst
    app/views/something_else/c.jst

Your templates become JST['something/a'], JST['something/b'] and JST['something_else/c'], which means your previously working JavaScript will likely fail because the names of templates have been changed out from under you.

I've created a patch that adds a configuration option called template_base_path that lets you override this behaviour and retain consistent template names by skipping over the logic in Jammit::Compressor#find_base_path. The option sets you define a common path based on ASSET_ROOT, allowing for the following sort of thing:

template_base_path: "app/views"

Using the above option, the previous template configurations would all become JST['something/a'], JST['something/b'] and JST['something_else/c'] regardless of the common paths and thus remain consistent even after adding or removing templates from the configuration. By default, template_base_path will be nil and the previous behaviour will be used. Tests are included and existing tests seem to pass.

jashkenas commented 13 years ago

Thanks for the patch -- it's a beautiful one.

However, I'm not entirely convinced that it's the correct way to tackle this issue. Having a mandatory path prefix is one thing, but if this is just an option, then you'll first have to be aware of the possibility of template names changing, in order to want to configure it in the first place ... and if you're aware of the possibility of template names changing, you're not going to adopt a directory structure which will cause this to be a problem in the first place.

I can understand how you'd want this option after getting bitten by the name change ... but for anyone else who runs into changing names, even if they then go and use this option, they still have to go rename the templates in their codebase.

dark-panda commented 13 years ago

Perhaps allowing for the use of a hash as an alternative to an array in the configuration would be an option, where the hash key is a constant name that you can refer to in JavaScript and the value is the path to the template. (Or vice versa, for that matter.)

You wouldn't be able to use globs for a path value in this case as they'd obviously be expanded and could refer to multiple templates, but you'd at least be able to supply more permanent template names and then change the paths as you need. I'd be willing to write up a patch for that if it sounds like a more suitable solution. Thoughts?

jashkenas commented 13 years ago

I'm just mostly wary of adding a configuration solution to a problem that doesn't exist if you think about your view directory structure up-front. Perhaps we just need to document this better:

Because JavaScript templates are named by their common path prefix, be sure to take a minute to think about how you'd like them to be organized.

dark-panda commented 13 years ago

You can think about your directory structure up-front all you want it would still take just a single new template to ruin all of your existing template paths. Granted, this isn't a huge thing, but it feels like a very fragile mechanism where a relatively small change like adding another template that just happens to be in another directory can ruin all of your other template names. The problem might not exist up front, but a simple refactoring like moving a template file should only affect things that directly refer to that file such as assets.yml and the places in code that you refer to that particular file. A change like that shouldn't cause all of these other files that happen to share a partial path to that same directory and all of the JavaScript files that refer to them to break as well.

I guess to me that it just feels like there's an extra bit of magic here that can gum things up unnecessarily. Thinking about it up-front is nice and all, but if even the best laid plans can be crippled by a directory path change, then there's something pretty fragile about that magic.

jashkenas commented 13 years ago

Yes -- I agree whole-heartedly. The current approach is definitely fragile. Here are the options:

dark-panda commented 13 years ago

Of those options, the only two that are really useful are the optional base-path configuration and the current approach, which I don't see as being completely mutually exclusive to one another (seeing as one of them is an optional addition to the other, and the behaviour defaults to the common-prefix approach anyways), but I would also expand on those descriptions with...

I'll think about an alternative though. There has to be a happy solution somewhere...