Restructure JS to be AMD-compatible #440
Conversation
convert tabs to spaces
This change makes Django-Debug-Toolbar use the existing version of jQuery on the page if one exists, and only loads jQuery if it isn't already loaded. It also explicitly loads the jQuery cookie plugin and the toolbar script as separate files. Finally, we are no longer compressing/minimizing scripts; this is for debug purposes, and presumably will not be served in production.
| @@ -3,7 +3,9 @@ | |||
| @media print { #djDebug {display:none;}} | |||
| </style> | |||
| <link rel="stylesheet" href="{{ STATIC_URL }}debug_toolbar/css/toolbar.min.css" type="text/css" /> | |||
| <script type="text/javascript" src="{{ STATIC_URL }}debug_toolbar/js/toolbar.min.js"></script> | |||
| <script>window.jQuery || document.write('<script src="{{ STATIC_URL }}debug_toolbar/js/jquery.js"><\/script>')</script> | |||
There was a problem hiding this comment.
can we use document.createElement('script')?
There was a problem hiding this comment.
For the <script> tag, I simply copied that line from the HTML5 Boilerplate project; it's less code than using JS to create a script element, attach the src attribute, and attach it to the DOM. I also think it's more readable this way.
jQuery would already be in the window if the Django project using Django Debug Toolbar was already loading jQuery -- something that I think is highly likely.
|
I'm not going to address issues with the Javascript itself -- those are perfectly valid, but outside the scope of this pull request. This pull request is solely about refactoring the existing Javascript to support AMD loading. Fixing issues about the Javascript itself should be tackled in a separate pull request. |
|
I can't figure out why my Travis build is failing; I've only changed HTML and JS, but the errors appear to be in Python logic. When I run |
to satisfy integration test
|
@cvan Alright, I fixed the test issue, and addressed your comment about using |
| script.src = "{{ STATIC_URL }}debug_toolbar/js/jquery.js"; | ||
| var exist = document.getElementsByTagName('script')[0]; | ||
| exist.parentNode.insertBefore(script, exist); | ||
| } |
There was a problem hiding this comment.
Would this be a problem if there are no <script> tags in the document?
In another solution I've done this before, the latter handling badly formed HTML (e.g. blank line at the top of the file):
(document.getElementsByTagName("head")[0] || document.documentElement).appendChild(script);
There was a problem hiding this comment.
There are at least three <script> tags in the document: these three here. :)
There was a problem hiding this comment.
But doesn't Javascript block document loading? If the <script> tag that this is in currently counts, then awesome. Works for me. It might be worth a few quick tests of abnormal HTML that this is injected into to make sure this change is good.
There was a problem hiding this comment.
As a test, I created a file with the exact contents <script>alert(document.getElementsByTagName('script').length);</script> and I opened it in Chrome, Firefox, and Internet Explorer 9. In all three, I got a popup alert with the number 1. I think it's fine. :)
There was a problem hiding this comment.
@singingwolfboy -- As noted yesterday, this turned out to be an issue in practice. An inline script runs immediately, while a script file runs once it's loaded. That's why your test didn't detect the problem.
I committed an old-school fix in 2b6f5d6. I don't know if it's compatible with AMD, but I need a way to work on the toolbar until we figure out a solution. If you have a better alternative, let me know!
EDIT: I just realized my "fix" reverts 62b2e37. We need something else.
|
lgtm |
Restructure JS to be AMD-compatible
|
Thank you! |
|
After this pull request, the example -- which you can run with The console says: |
When jQuery wasn't loaded yet, the <script> tag was inserted properly, but jQuery wasn't loaded before jquery.cookie.js and toolbar.js. As a consequence these scripts crashed. Refs #440.
|
This is causing problems for me when I'm using require.js for other purposes on the page. Isn't it better for the debug toolbar javascript to be completely isolated from the application? |
|
I use AMD on my projects, but this change actually caused it to break in AMD environments. I am curious as to why AMD compatibly was desired in DDT. DDT isn't using an AMD loader itself and is just using multiple script tags (which is against the point of AMD). Using AMD like this could actually conflict with plugins like in #525. In my opinion, DDT should be self contained and stay out of the way of any code in the page. Having DDT use RequireJS/Dojo/another AMD library if it's in the page can introduce more issues with little upside. I don't mean to seem non grateful for the work done by all the contributors to this item, but I think AMD-compatibility doesn't make sense for DDT. |
|
I have the same question : why is DjDt amd-aware ? What's the rational behind this ? oõ |
Restructure JS to be AMD-compatible
When jQuery wasn't loaded yet, the <script> tag was inserted properly, but jQuery wasn't loaded before jquery.cookie.js and toolbar.js. As a consequence these scripts crashed. Refs django-commons#440.
This pull request refactors the Javascript a bit to make it AMD-compatible. It also drops JS compression, because this is a debug toolbar, and there's no reason to compress assets that are only going to be used in debugging -- in fact, if someone discovered a problem with an interaction between the Django Debug Toolbar and their Javascript code, having compressed JS is only going to make it harder to debug. I also removed the
jQuery.noConflict(true)call from toolbar.js in favor of only loading the jQuery library if it hasn't already been defined on the page, which should solve the same problem more neatly.This diff contains a fair number of whitespace-only changes in toolbar.js; to ignore those changes when viewing the diff on Github, you can add
?w=1to the URL. I find that makes pull requests like these easier to review.