Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#12882 closed Uncategorized (fixed)

jQuery.noConflict() in admin breaks site specific code with jQuery

Reported by: krejcik@… Owned by: jezdez
Component: contrib.admin Version: master
Severity: Normal Keywords: jQuery admin
Cc: ales.zoulek@…, yed@…, rob@…, semente@…, michael.c.strickland@…, gabrielhurley Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by jezdez)

JQuery javascipt included by django admin calls noConflict method ( http://api.jquery.com/jQuery.noConflict/ ). This method removes variable $ shortcut from the global namespace.
Site specific code can include an own jQuery code to enrich admin pages. In this case, commonly used variable $ is undefined and page is broken.

Attachments (3)

12882.1.diff (858 bytes) - added by jezdez 4 years ago.
Pass true to noConflict as mentioned on http://api.jquery.com/jQuery.noConflict/
12882.2.diff (1.4 KB) - added by jezdez 4 years ago.
The collapse widget needs jQuery, too
12882.3.diff (11.0 KB) - added by jezdez 4 years ago.
Moved admin jQuery into our own namespace to lower the risk of clash with third party apps that use jQuery.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 4 years ago by jezdez

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by anonymous

  • Cc ales.zoulek@… added

comment:3 Changed 4 years ago by yedpodtrzitko

  • Cc yed@… added

comment:4 Changed 4 years ago by jezdez

I'm not sure what you are saying with ".. causing problem with strip of jQuery plugins after redefinition". Mind elaborating?

comment:5 Changed 4 years ago by jezdez

  • Owner changed from nobody to jezdez
  • Status changed from new to assigned

comment:6 Changed 4 years ago by ales_zoulek

jezdez: Admin's jquery.min.js may overide some site-specific javascript. And since there are tons of jQuery plugins, versions and possible combinations, (and I suspect a lot of more customized admin sites uses jQuery) site should be always able to enforce it's jQuery version (and thus be responsible for and core disfunctionalities it may cause).

At the moment, overwriting default jquery is possible either by:

  • rewring ModelAdmin.media property (in every admin option class, or by monkeypatching)
  • copy'n'pasting templates/admin/change_list.html and changing only "extrahead" block

Extending "extrahead" block in base.html or site_base.html is not possible since change_list.html writes it's media (including jquery) AFTER {{block.super}}.

Possible solutions would be one of:

  • Including all admin javascripts in base.html (What is the dynamic contruction of list of internal JS files to include on every page good for, anyway? :) )
  • Introduce {% block customjavascript %} block tag in admin/base.html that would go after {% block extrahead %} since there's no easy way to put custom tags at the end of extrahead block.

Workaround:

  • Extend {% block blockbots %} tag in custom base_site.html and put custom jQuery there.

comment:7 follow-up: Changed 4 years ago by krejcik

about plugins, see symbolic example
<head>

<script JQuery by Site>
<script Plugin 1 (by site)>

<script JQuery by Django>
<script Plugin 2 (by Django)> (eq. actions.min.js)

<head>

In example above JQuery by Django redefine jquery instance, so Plugin 1 is not defined on new jQuery instance. For example it can be solved by ommiting first jQuery and putting Plugin 1 include after Plugin 2. (But jquery by django is included in media part which is last in head, so it needs some work to override it (especially if it can be generic solution for whole project and not only one template))

In fact it is not main topic of this ticket, but i would like to show more issues with jQuery include by Django. I my opinion is not a bad idea use this great library, but O guess it should be done in more customizable way and with respect about site which can use own jquery (maybe in worst case in different version) with it's own jQuery plugins. Present situation must be hacked by ugly workarounds in client code.

comment:8 Changed 4 years ago by krejcik

And finally my workaround for noConflict issue is

jQuery.noConflict = function() { return this; }; {# HACK - Django calls it #}

in right place in html document.

comment:9 in reply to: ↑ 7 Changed 4 years ago by jezdez

Replying to krejcik:

In fact it is not main topic of this ticket, but i would like to show more issues with jQuery include by Django. I my opinion is not a bad idea use this great library, but O guess it should be done in more customizable way and with respect about site which can use own jquery (maybe in worst case in different version) with it's own jQuery plugins. Present situation must be hacked by ugly workarounds in client code.

Absolutely, that's why we are here, fixing that bug :)

comment:10 Changed 4 years ago by robhudson

  • Cc rob@… added

It is possible to completely encapsulate Django's jQuery version and its plugins, but I'm curious if that's the best thing to do here. If it's known that the Django admin has jQuery available via jQuery in the global namespace, custom admin site changes can code to that. I, for one, like knowing I can use Django's version of jQuery for some custom widgets and such.

However, if we do want to encapsulate things so different versions of jQuery can co-exist, it is possible following this skeleton code:

(function(window, document, version, callback) {
	var j, d;
	var loaded = false;
	if (!(j = window.jQuery) || version > j.fn.jquery || callback(j)) {
		var script = document.createElement("script");
		script.type = "text/javascript";
		script.src = "/path/to/jquery.js";
		script.onload = script.onreadystatechange = function() {
			if (!loaded && (!(d = this.readyState) || d == "loaded" || d == "complete")) {
				callback((j = window.jQuery).noConflict(1), loaded = true);
				j(script).remove();
			}
		};
		document.documentElement.childNodes[0].appendChild(script);
	}
})(window, document, "1.4", function($, jquery_loaded) {
	// plugin code goes here.
});

What this code does is checks if there is an existing version of jQuery loaded and checks it meets a given minimum version and if so, uses that jQuery. If it doesn't exist, or doesn't meet the minimum version, it dynamically adds a script tag to load its own jQuery and uses that. The version loaded is encapsulated within the anonymous function and will not conflict with any other Javascript libraries (e.g. other versions of jQuery, prototype, etc.).

It seems a bit overkill to me, but if it's the best option, feel free to use it.

-Rob

comment:11 Changed 4 years ago by krejcik

Because jQuery has stable API, which is almost backward compatible, I agree that solving different version seems to be overkill.
Personaly I see problem only in noConflict call and in jQuery instance replacement by another JS include (which cause lost of previously defined jQuery plugins as desciribed in a previous post).

Changed 4 years ago by jezdez

Pass true to noConflict as mentioned on http://api.jquery.com/jQuery.noConflict/

comment:12 Changed 4 years ago by jezdez

In the attached patch I'm referring to the part of jQuery's documentation:

"If necessary, we can free up the jQuery name as well by passing true as an argument to the method. This is rarely necessary, and if we must do this (for example, if we need to use multiple versions of the jQuery library on the same page), we need to consider that most plug-ins rely on the presence of the jQuery variable and may not operate correctly in this situation."

Question is, can we test this somehow? Does it solve your issue, krejcik?

Changed 4 years ago by jezdez

The collapse widget needs jQuery, too

comment:13 Changed 4 years ago by taylormarshall

"In this case, commonly used variable $ is undefined and page is broken."

krejcik: can't this be solved by just doing wrapping your code in a function? Like so:

(function($) {
   // your jQuery code goes here.
})(jQuery);

I do this very commonly in my own code, just to ensure that if someone decides to use another library or screw with the $ variable (not that uncommon on large projects), my scripts can still be reused just fine when jQuery is in noConflict mode.

My apologies if I am missing something about why this wouldn't work.

comment:14 Changed 4 years ago by krejcik

taylormarshall: yes it can be used in this way. but I guess main problem is in jQuery plugins. (after second jQuery include, previosly included plugins are lost

comment:15 Changed 4 years ago by Guilherme Gondim (semente) <semente@…>

  • Cc semente@… added

comment:16 Changed 4 years ago by Guilherme Gondim (semente) <semente@…>

  • Cc semente@… added; semente@… removed

comment:17 Changed 4 years ago by gabrielhurley

  • Cc gabrielhurley added

comment:18 Changed 4 years ago by jezdez

  • Description modified (diff)
  • Summary changed from jQuery.noConflict() in admin brokes site specific code with jQuery to jQuery.noConflict() in admin breaks site specific code with jQuery

comment:19 Changed 4 years ago by ramiro

  • Has patch set

comment:20 Changed 4 years ago by jezdez

  • Patch needs improvement set

I'm not sure yet if the 12882.2.diff patch is enough and the right approach. I've been playing with the idea of making the shipped jQuery live in a namespace, as in

var django = {};
django.query = jQuery.noConflict();
Last edited 3 years ago by jezdez (previous) (diff)

comment:21 Changed 4 years ago by jezdez

  • Needs tests set

comment:22 Changed 4 years ago by anonymous

  • Cc michael.c.strickland@… added

comment:23 Changed 4 years ago by lukeplant

jezdez: Use of jQuery.noConflict(true) will remove jQuery from the global namespace, which means that any jQuery call that appears after that line on the page will stop working. For example, if you have both a stacked inline and a tabular inline, the second stops working since the first removed jQuery (I've tested that).

So, the options left are:

  • provide a mechanism to stop multiple instances of jQuery being loaded
    • either based on overriding template blocks
    • or something like Rob Hudson's code
  • your django namespace idea.

comment:24 Changed 4 years ago by gabrielhurley

Personally I'm a fan of namespacing Django's copy of jQuery.

That way developers who want to use the vanilla admin jQuery can do so with a reliable, stable namespace for it while those who want their own copy, own plugins, etc. can do so conflict-free. That strategy would work well with the custom admin code I have written currently and seems like a more maintainable mechanism in the long run.

Preventing loading of multiple jQuery instances via template blocks sounds messy and likely to be broken by people hacking at the admin.

Rob Hudson's code definitely works but still feels brittle to me, like if someone includes their script in the wrong place. I'd rather see something more robust and separate.

That's my $0.02 for what it's worth.

comment:25 Changed 4 years ago by crucialfelix

++1 on django's admin keeping its own jquery isolated in its own namespace

the admin is already too brittle and has by far taken up more of my development time than any other aspect of working with django. tracking the media includes (with inlines!) is a real chore just to identify who still is including jquery.

comment:26 Changed 4 years ago by lukeplant

With regards to the original bug here, I don't think we have got to the bottom of it. jQuery.noConflict() is supposed to restore the original definition of $, which has been saved in a global variable _$ when jQuery initializes. I can see that this may fail if 3 or more instances of jQuery are loaded, but it should work correctly with 2.

If we go for the Django namespace, then the call should be:

django.jQuery = jQuery.noConflict(true);

...so that the original jQuery is restored. All code/plugins will need to be updated for this, but in all cases so far it should be a minimal change - it needs to look like this:

function($) {
  ...
}(django.jQuery);
Last edited 3 years ago by jezdez (previous) (diff)

Changed 4 years ago by jezdez

Moved admin jQuery into our own namespace to lower the risk of clash with third party apps that use jQuery.

comment:27 Changed 4 years ago by jezdez

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12966]) Fixed #12882 - Moved the admin's jQuery into our own namespace to lower the risk of a clash with third party apps that use jQuery.

comment:28 Changed 3 years ago by jezdez

In [16415]:

Stopped the admin JavaScript code from completely removing the jQuery object from the global namespace to ease the use of jQuery plugins. The django.jQuery object is still supposed to be used by Django's own JavaScript files. Refs #12882.

comment:29 Changed 3 years ago by anonymous

  • Easy pickings unset
  • Resolution fixed deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

The problem with [16415] is that if I include my own jQuery in the admin's extrahead block, which the jQuery copy shipped by the admin overwrites, the admin will only restore window.$ and won't restore window.jQuery. This breaks things like django-selectable (which expects the jQuery UI plugin to be loaded on the jQuery object), and most jQuery code which lives in a jQuery(function($) { ... }); scope.

This should be fixed by reverting [16415] and requiring jQuery users to bring their own jquery script tag; admin-only addons can still use the django.jQuery(function($){ ... }); idiom.

comment:30 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from reopened to closed

Please don't reopen old tickets but open new ones for issues that have been fixed.

comment:31 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.