Posted by Gábor Hojtsy on November 18, 2010 at 1:06pm
4 followers
Jump to:
| Project: | Wysiwyg |
| Version: | 7.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
In #769226: Optimize JS/CSS aggregation for front-end performance and DX, the drupal_add_js() API was changed to take JS_* constants as 'group' not as part of 'weight'. 'weight' and 'group' are now separate. Attached very simple patch to implement this.
| Attachment | Size |
|---|---|
| add_js_group.patch | 1008 bytes |
Comments
#1
Before this, wysiwyg.init.js was loaded after all the core scripts in misc/, with the patch it's loaded right after drupal.js and before vertical-tabs.js, states.js and form.js but given its contents this is not a problem.
Patch looks good and does what it's supposed to.
I'm wondering if we shouldn't also put the editor libraries in the JS_LIBRARY group as well (in
wysiwyg_load_editor()). My initial tests with this seems to work fine, though I wonder what happens if a library decides to overwrite $ or something like that. I know openWYSIWYG does that so I made a quick test with it and CKEditor. Didn't notice any issues so far. (Wysiwyg's openwysiwyg.js resets the global $ back to jQuery and wraps openWYSIWYG's methods to use its version of $, but it's loaded long after openWYSIWYG's files.) Given the D7 JavaScript guidelines recommends using the(function ($){...})(jQuery);wrapper, this shouldn't be a problem for Drupal modules at least. Third party scripts not using such a wrapper might not be so fortunate.#2
Well, this bug is about the basic API change that JS_* constants should now be used for 'group', not for 'weight'. Glad it tested well :) We run the module with this patch for the past two weeks, and it worked out for us nice as well.
#3
Thanks for reporting, reviewing, and testing! Committed to HEAD.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
#4
Automatically closed -- issue fixed for 2 weeks with no activity.