meleyal / backbone-on-rails

A simple gem for using Backbone.js with Rails.
MIT License
809 stars 125 forks source link

Allow namespacing for generators #7

Closed JeanMertz closed 12 years ago

JeanMertz commented 12 years ago

This commit adds some options to the generators:

rails generate backbone:install

Usage:
  rails generate backbone:install [options]

Options:
  -j, [--javascript]           # Generate JavaScript
  -n, [--namespace=NAMESPACE]  # Namespace backbone application in specified directory tree
  -m, [--manifest=MANIFEST]    # Javascript manifest file to modify (or create)
                               # Default: application.js

Runtime options:
  -f, [--force]    # Overwrite files that already exist
  -p, [--pretend]  # Run but do not make any changes
  -q, [--quiet]    # Supress status output
  -s, [--skip]     # Skip files that already exist

Generates a Backbone.js skeleton directory structure and mainfest

rails generate backbone:scaffold

The scaffold generator now accepts both posts and admin/posts as NAME, this results in a namespaced scaffold.

JeanMertz commented 12 years ago

This fixes #1

meleyal commented 12 years ago

Thanks, this is great, I just got around to testing it.

I would just make one change: the namespace should be a property of the App object, so SpaceApp.SolarSystem instead of SpaceAppSolarSystem.

derekprior commented 12 years ago

@JeanMertz @meleyal Any hope in seeing this merged? I too use backbone for just a small portion of my site and would love to have the generators create namespaced assets.

meleyal commented 12 years ago

Yes, this is definitely planned. I would rather create a true nested namespace for the app object (App.Namespace vs AppNamespace), but it gets complicated with multiple namespaces, so maybe this is good enough for now.

I will look at merging this when I get a minute.

JeanMertz commented 12 years ago

I'm sorry for not getting back to you guys earlier (busy and all). I thought about using dots between namespaces, but like you said, it can get complicated with multiple namespaces.

If anyone can easily change this, I'm all for it, as it's a lot cleaner than CamelCasing the namespaces.

edit: I also just rebased the pull request with your latest changes, so it should be easy for you to pull this in whenever you feel like it.

JeanMertz commented 12 years ago

I added some more tweaks to this pull request. I think they aren't too extensive to warrant another pull request. However, if you feel some changes aren't what you want for this gem, I'll just leave them out of the pull request and use my fork.

meleyal commented 12 years ago

Thanks a lot. I cherry picked everything but 4387066 (sticking to the thoughtbot conventions) + a6346a8 (could result in broken JS out of the box e.g. App.Namespace = App is undefined)

Now to write those unit tests...

JeanMertz commented 12 years ago

Great to see this getting pulled in. I haven't really tested my last commit, so if you say namespacing like that won't work I'll take your word for it.

Regarding the naming conventions:

Just my two cents.

meleyal commented 12 years ago

I don't have access to the full ebook (too broke) so I was just going on the sample chapter: http://ui.thoughtbot.com/assets/backbone-js-on-rails-thoughtbot-ebook-august-2011-sample.html#_rails_3_1

But I'll take your word for it :) fb0cf36cb33651e97e5494a70df80bb7f372f49f

I also updated the docs with the new options: https://github.com/meleyal/backbone-on-rails#readme

JeanMertz commented 12 years ago

Well, I guess you are right about the sample chapter. I asked the thoughtbot guys in an issue on their ebook repo to clarify how they would order things so you can revert (or keep) the change once I hear back from them.

Also, could you elaborate on how/when the namespacing using dots won't work, and how to solve that problem? I really like the App.Namespace approach compared to AppNamespace, but if it causes problems in the long run, I guess I'll have to settle with the AppNamespace one

Anyway, thanks for all the updates!

meleyal commented 12 years ago

In the case of App.Namespace, if App is not already defined (e.g. with window.App = {}), you will get an error.

So for multiple namespaces, we just have to make that each one is defined before we 'append' to them.

JeanMertz commented 12 years ago

I see...

So in order for this to work, for example for the App.Backend.Admin namespace, you'd have to set:

window.App = {}
window.App.Backend = {}
window.App.Backend.Admin =
  Models: {}
  Collections: {}
  Views: {}
  Routers: {}

But then, one of the reasons to namespace your Backbone app is so that you can have multiple backbone apps tied to one rails app. So then if you'd also want a separate Backbone app for example for App.Backend, you could get into trouble with overriding other namespaces.

In other words, this could get very messy if you don't know what you are doing, as you already said.

Maybe it is best to keep the current namespacing then. If you want it the way we described above, you can always change the code yourself, it's not like something magical happens, it's just a few lines of code.

The only thing you could do, is to provide a boolean option for the scaffold generator to use dot-notation instead of camelcasing the namespaces. But I guess if you know your way around javascript/backbone, you'll probably never use the scaffolding...

meleyal commented 12 years ago

You could do something like this to avoid the overriding problem:

window.App = window.App || {};
App.Backend = App.Backend || {};
App.Backend.Admin = {
  Models: {},
  Collections: {},
  Views: {},
  Routers: {}
};

But yes, it gets a bit messy.