-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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.)
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! |
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? |
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. |
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. |
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. |
You're right @frewsxcv, keeping a separate copy of jQuery isn't ideal. DJDT used to use |
Fix django-commons#581, django-commons#544, django-commons#541, django-commons#535 and a few others.
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.djdtjQuery
to avoid the need to usenoConflict
inbase.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.