generate admin.js as part of gem task #90

Closed
johnmuhl opened this Issue Feb 8, 2010 · 14 comments

Projects

None yet

3 participants

asmala commented Mar 30, 2010

I created a rake task to create the cached all.js and hooked it up with the existing radiant:update task. I emailed the patch to the radiantcms-dev list, but since it was my first post, it's still held for moderation. In the mean time I posted the diff as a gist.

Owner

Is there a way to do this without keeping a list of files to join in 2 places? (the layout file and the rake file)

asmala commented Mar 30, 2010

Yeah, I also felt it wasn't quite right to declare the list twice. I was considering some alternatives but the other options didn't seem particularly elegant either:

  • A) We could rename the JS files with a numeric prefix (1-prototype, 2-effects, etc.) so the files would load in correct even without setting them up in an array. I think.
  • B) We could declare the JS files array as a constant somewhere.
  • C) Invoke something like webrat in production mode to trigger the creation of all.js.
  • D) We could have the layout only include all.js, independent of environment.

Option A strikes me as a bad way to manage the load order, e.g. what happens when want to add a new JS somewhere in the queue?

Option B and C seem like overkill for an array that only gets used once outside the layout.

Option D doesn't look like an ideal solution either, considering the inconvenience caused for Radiant development.

Owner

why not just read the list out of views/layouts/application.html.haml?

Owner

That sounds good to me

asmala commented Mar 30, 2010

sheepish grin

Yes, that makes a whole lot of sense. Let me see if I could what it together soon.

asmala commented Mar 30, 2010

How about this updated version? I incorporated the suggestion from johnmuhl and brought it in line with edge. It could use some more robust error handling (e.g. what if the JS line in layout gets changed), but perhaps that's not entirely necessary.

Owner

asmala,

Thanks very much for tackling this. I'm inclined to move this into http://github.com/radiant/radiant/blob/master/lib/task_support.rb and write specs for it http://github.com/radiant/radiant/blob/master/spec/lib/task_support_spec.rb

Then they could be written similar to this http://github.com/radiant/radiant/blob/master/lib/tasks/radiant_config.rake

I'm inclined to push all tasks into this since I can personally think of 2 times where this would have saved us from bugs, 1 of which made it into an actual release and had to be fixed shortly thereafter.

Would you be able to do that too?

asmala commented Apr 3, 2010

That does seem like a more robust way of going about this. I like it. I'll try to whack it together over the weekend, quite possibly very soon.

asmala commented Apr 3, 2010

That turned out to be fairly easy, although the set up for the file caching spec could be more elegant.

Also, I noticed that we've got quite a few path names using slashes (e.g. #{Rails.root}/config/database.yml). Now I haven't been using Windows for quite a few years, but wouldn't that cause a problem on platforms that use other characters as a directory delimiter? If this is the case, I can open a new issue to recommend moving to using File.join instead.

Owner
johnmuhl commented Apr 3, 2010

the slashes should be handled by ruby and turned into system appropriate ones when used properly.

Owner

I've committed Janne's changes in 3816f81
Thanks very much for that!

I just took a quick look at the message thread in the original issue and Sean mentioned this being a part of the "gem task" which I assume would be radiant:gem
So I'll leave this open until that's done too... if anyone wants to tackle that, it would be really helpful. Otherwise it's on my list of things to do.

asmala commented Apr 6, 2010

I'm not too familiar with gem packaging, but this might work. I further abstracted the admin JS caching and created a task that I hooked up to the radiant:gem task.

Owner

generate all.js from rake radiant:gem task. closed by 86bea01

@rx rx pushed a commit to voomify/radiant that referenced this issue Jul 19, 2011
@johnmuhl johnmuhl generate all.js from `rake radiant:gem` task. closes #90 25b11a3
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment