Skip to content
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

Changed "instanceof jQuery" to "instanceof $" #422

Closed
wants to merge 1 commit into from

Conversation

ilyasfoo
Copy link

@ilyasfoo ilyasfoo commented Sep 2, 2014

Original code will break if jquery noConflict is applied, where $ and jQuery produces instance of different versions.

Original code will break if jquery noconflict is applied, where $ and jQuery produces jQuery instance of different versions.
@ericmann
Copy link

When jQuery.noConflict() is invoked, it removes the alias of $ and leaves it undefined. If you have both jQuery and $ working and giving you different versions, then it means you have two versions of jQuery loaded on the page. That's a bug in your particular implementation.

That said, if jQuery.noConflict() is invoked and another version of jQuery is not loaded, then $ will be undefined and this code will break. I highly recommend not making this change.

@dsmorse
Copy link

dsmorse commented Apr 8, 2015

Thanks, I merged this into my fork dsmorse/gridster.js

@matthewbpt
Copy link

I was about to make a pull request for this exact same issue.

@ericmann This won't break because in this case $ is not a global, it's the jQuery object passed into the factory function, see here. If calling jQuery.noConflict() causes this code to break then the whole library will break as there are many other references to $ in the file. There is no need to use the global jQuery object as we are passing it into the factory function.

I recommend this change is made as it will make the code consistent as all references to jQuery will be the same.

@matthewbpt
Copy link

@ericmann
Copy link

This won't break because in this case $ is not a global, it's the jQuery object passed into the factory function.

@matthewbpt While that might be true, my original point remains. If $ and jQuery are reporting two different versions (as was stated in the original justification for this patch) then there are deeper problems with the site.

@ilyasfoo
Copy link
Author

Yes your point is valid. I have two versions of jQuery in my site because of several old plugins that I require does not support the latest version of jQuery. This is an edge case but I think some other users are having the same problem too.

However, I believe my pull request is still useful because it provides consistency in gridster. In all instances, gridster use $, except for Line 842 where it suddenly uses jQuery:

Line 725:
var $el = el instanceof $ ? el : $(el);

Line 842:
var isDOM = $el instanceof jQuery;

Line 2531:
if (arguments[1] instanceof $) {

Line 2615:
if (arguments[2] instanceof $) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants