Skip to content

Always load a separate copy of jQuery w/ AMD compatibility #544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Conversation

djfarrelly
Copy link
Contributor

This build's on #541, but adds fixes functionality for use along side AMD projects. With DjDT's current module structure, the only way to truly support this is modifying a copy of jQuery itself to add a namespace:
#541 used added a namespace for DjDT's version of jQuery. jQuery's inner define call (see source) still registered jQuery as 'jquery'. We change this to "djdt.jquery" so DjDT modules like toolbar.js line 4 will load that.

Additionally on line 9110 of our modified jQuery, I assigned jQuery to window.djdtjQueryto avoid the need to use noConflict in base.html.

Note: I know modifying the jQuery source may be viewed as something not to do, but for the time being, this will make sure that DjDT's jQuery does not conflict with the end user's jQuery or an AMD project. In my opinion, the best way to avoid all issues across the board would be to refactor DjDT without jQuery, but that would take some time.

PS - I'm not sure if I made this pull request correctly to build on and cite @aaugustin's PR (#541). I didn't mean to duplicate his work, only amend it.

aaugustin and others added 2 commits February 9, 2014 10:09
I have no idea what this does for AMD / RequireJS users. No one is able
to explain to me how AMD / RequireJS work. I hope this commit doesn't
make the situation significantly worse, but really I can't tell.

Fix #535. (Well, not really, but it'll allow me to close the ticket.)
@djfarrelly djfarrelly mentioned this pull request Feb 9, 2014
@aaugustin
Copy link
Contributor

Thanks for working on this. I'll review your commit within a few days.

FYI we recently stopped shipping jQuery in order to keep the DDT package small. It accounted for more than two thirds of the download size!

@djfarrelly
Copy link
Contributor Author

Ok, I see why jQuery was removed, maybe my PR isn't the right move then. jQuery has added custom modular builds where we could create a build with only what DjDT needs. That could be an option.

Alternately, after reviewing DjDT's javascript earlier today, the project could definitely be refactored without jQuery. It would probably add to the size of the javascript a bit, but the project would have zero front-end dependencies which is great. @aaugustin, is this something you would consider if a pull request was made? Or do you want to keep jQuery part of the project since it's API is so widely known?

@aaugustin
Copy link
Contributor

Well, this may break third-party panels, but overall it could be a good move. I'm all for "no-deps" in general... If you're ready to work on this (which is much appreciated) can you open a new issue? I'll try to get feedback from the other maintainers.

@djfarrelly
Copy link
Contributor Author

Ah, I see one or two of the panels (linked to the docs) use jquery, so even this namespacing jquery could break them. Another benefit of "no deps" would be that it would consistently work when the user is offline. Anyway, I'll look into it more and if I can put some time into it, I'll open a new separate issue. Thanks.

@frewsxcv
Copy link
Contributor

It would be ideal if we didn't have to modify the jQuery source everytime we need to upgrade jQuery versions. I haven't tested this, but you might be able to use jQuery.noConflict() in this situation:

<script src="{% static 'debug_toolbar/js/jquery.js' %}"></script><!-- include unmodified jQuery source -->
<script>
window.djdtjQuery = $.noConflict(true);
// setup requirejs djdt.jquery module here
</script>

It seems like the breaking of third-party panels is unavoidable. Just make sure to document it well in the changes.

@djfarrelly
Copy link
Contributor Author

You're right @frewsxcv, keeping a separate copy of jQuery isn't ideal. DJDT used to use $.noConflict(). A lot of these issues have been discussed in #535. The best solution might actually be to use an iframe like @AdamCraven recommended.

aaugustin added a commit that referenced this pull request Apr 25, 2014
Fix #581, #544, #541, #535 and a few others.
@aaugustin aaugustin closed this Apr 25, 2014
ryneeverett pushed a commit to ryneeverett/django-debug-toolbar that referenced this pull request Oct 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants