This project is archived and is in readonly mode.
Allow to register javascript/stylesheet_expansions to existing symbols
register_javascript_expansion and register_stylesheet_expansion currently just merge the given expansions to the respective class variables, thus overwriting previously registered symbols.
The "register" naming of these methods kinda suggests that this is possible IMO. Also there's no other accessor (anymore) that would allow subsequent registrations from multiple plugins.
The attached patch makes it so that you can register multiple javascripts/stylesheets in subsequent calls and they get merged:
def test_custom_javascript_expansions_merge_with_existing_symbols
["head", "body", "tail"].each do |script|
ActionView::Helpers::AssetTagHelper::register_javascript_expansion :monkey => [script]
end
assert_dom_equal %(<script src="/javascripts/first.js" type="text/javascript"></script>\n<script src="/javascripts/head.js" type="text/javascript"></script>\n<script src="/javascripts/body.js" type="text/javascript"></script>\n<script src="/javascripts/tail.js" type="text/javascript"></script>\n<script src="/javascripts/last.js" type="text/javascript"></script>), javascript_include_tag('first', :monkey, 'last')
end
-
DHH- Milestone set to 2.x
-
CancelProfileIsBroken- Tag changed from 2.3, actionview, expansions, javascript, patch, stylesheets to 2.3, actionview, bugmash, expansions, javascript, patch, stylesheets
-
Dan Pickettverified that this applies cleanly to 2-3-stable
-1 on this, though imo - this could get really confusing and unwieldy if you keep appending to the same expansion - the constraint of the one expansion per call keeps things clean
-
Rizwan Reza-1 on this one.
-
Simon JeffordVerified but -1 on this. I'm struggling to think of a use for this.
-
Jeremy Kemper- State changed from new to open
Use case: plugin wants to add a js to head.
This is technically a backward-incompatible change, though, so not appropriate for 2-3-stable.
-
Elad Meidar-1 We would you want subsequent calls to register assets ? i assume as a developer, that i would like to see only one place where each plugin registered assets expansions keys.
also, patch does not apply on master.
-
Jeremy Kemper
It's so plugins can share keys.
-
Dmitry Ratnikov
Jeremy, do you happen to have an example why would plugins need to share keys? Plugin authors should be able to make their asset namespace unique by using the plugin name (e.g. :acts_as_monkey => [ 'tail', 'body', 'head' ]).
The patch does not apply cleanly to master either.
-
Jeremy Kemper
- Tag changed from 2.3, actionview, bugmash, expansions, javascript, patch, stylesheets to 2.3, actionview, expansions, javascript, patch, stylesheets
- State changed from open to wontfix
I don't have an example. Hence wontfix :)
-
Sven Fuchs- Assigned user set to Jeremy Kemper
- State changed from wontfix to open
Tss .. :)
Examples are probably coming from the Engines world. Think of two inhouse engines that are supposed to hook into a common admin backend. The admin backend layout is somewhere else (maybe in another engine) and contains the javascript/stylesheet assets helper calls. Maybe the two engines are a blog and a forum engine and they both ship js/css assets that are relevant for the admin backend.
When you install the forum engine it should be able to transparently register its js/css assets. You should not need to change the admin layout for that. The obvious solution to this is to just add onto an existing expansion without clearing it.
Can this be solved more unobtrusively with an additional option? like :really_just_register_without_killing_other_peoples_stuff => true? or just :append => true
Jeremy, am I supposed to reopen the ticket now? I'll just do that :)
-
-
Sven Fuchs
- Importance changed from to
bump
We still need this :) And it still makes so much sense. Even more with the advert of super-powerful engines in rails 3.
-
Sven Fuchs
bump :)
I've pushed a patch to 3-0-stable here: https://github.com/svenfuchs/rails/commit/b712476ac3f20d1e976ea431c... (I currently can't seem to upload/attach patches to lighthouse tickets, but I guess a fork commit will do as well?)
Folks, the usecase for this is rather straight forward and obvious for people who use engines to split up applications.
Maybe think of a bunch of engines that provide a blog, tickets, newsletter, ... whatever. Now they'll all contribute routes, controllers, views, assets to a common administration backend and the shared admin layout itself is provided by an engine.
Engines should now be able to register their assets (e.g. admin/blog.css, admin/tickets.css, ...) to a shared asset expansion which then can be used by the admin layout. Depending on what engines are installed the layout will include the relevant assets.
Believe me, it's the right way to do this :)
Obviously one also could monkey-add another helper method, but "registering" already means, well, registering and not overwriting. So this custom helper method maybe would be :really_register_stylesheet_expansion?
This change won't break anything for people who use asset expansions in a single place in their application. But it allows others to add on top of them.
-
Deleted User- Tag changed from 2.3, actionview, expansions, javascript, patch, stylesheets to 2.3, 3.x, actionview, expansions, javascript, patch, stylesheets
+1
:engine_admin would be the perfect expansion example for engines which solve different, distinct problems, but all have a common admin-interface.
I was really surprised that this didn't work.
-
Clemens KoflerWouldn't this make sense in a style of before/after filters, view paths, middleware etc. in the sense that you can prepend, append and insert before/after some other JS? My thinking is that in JavaScript most of the time load order is important (e.g. jQuery before anything that uses jQuery eagerly).
-
Sven Fuchs
Hey Clemens, well, so far people didn't even see any usecase here :)
On the one hand one could argue that engines shouldn't need to know that much about each other. So they could register their own dependencies (like jquery).
On the other hand engines already register initializers in the same way you mention.
This stuff works fine for us right now (it's actually been extracted from adva-cms2), so I'd wait for people coming up with the need for a more powerful mechanism. Btw, these could easily be extra helper methods (like :prepend_registration or something ...)
-
Josh Kalderimis- State changed from open to resolved
Hi Sven,
Merging of expansion is in both 3-0-stable and master, so I am closing this issue as resolved.
Thanks,
Josh
-
Sven Fuchs
Hey Josh,
yeah, thanks!
Apparently Aaron has applied the patch meanwhile https://github.com/rails/rails/blob/55b13c532f8c1dbbe34695f356c62fe...
Sorry for not paying more attention here :)
-
Josh Kalderimis
Hey Sven,
No need to apologize, and its my pleasure, thanks for pushing and pushing for this change.
Have a great weekend.