ducksboard / gridster.js

gridster.js is a jQuery plugin that makes building intuitive draggable layouts from elements spanning multiple columns
http://gridster.net/
MIT License
6.04k stars 1.2k forks source link

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

Closed ilyasfoo closed 3 years ago

ilyasfoo commented 10 years ago

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

ericmann commented 9 years ago

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 commented 9 years ago

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

matthewbpt commented 8 years ago

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 commented 8 years ago

The change has already been made in many other parts of the code, see:

https://github.com/ducksboard/gridster.js/blob/master/src/jquery.gridster.js#L725 https://github.com/ducksboard/gridster.js/blob/master/src/jquery.gridster.js#L2531 https://github.com/ducksboard/gridster.js/blob/master/src/jquery.gridster.js#L2615

It should definitely be made here too.

ericmann commented 8 years ago

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 commented 8 years ago

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 $) {