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

Removing jQuery dependency for Bloodhound #163

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jslegers
Copy link

@jslegers jslegers commented Oct 6, 2017

I removed jQuery as a dependency for Bloodhound.

Implementation details

  • I replaced $.error(s) with new Error(s)
  • I replaced $.deferred with simply-deferred, which introduces a Deferred object that has an API that's compatible with $.deferred
  • I replaced $.ajax with a custom implementation that partially implements the API of $.ajax and returns a Deferred object
  • I replaced $.extend, $.noop, $.param and a few other methods with equivalent methods in common/utils that have the same API and the same behavior (at least in the contexts required by Bloodhound & Typeahead) but don't use jQuery as a dependency
  • I replaced all util methods in common/utils that used jQuery as a dependency with equivalent methods that have the same API and the same behavior but don't use jQuery as a dependency

Caveats

  • Unit tests still use $.ajax instead of _.ajax because ajax mocking in unit tests relies on jasmine-ajax v1.3.1, which only supports Ajax calls that use jQuery.
  • Although no unit tests are failing, I'm using Array.isArray quite a few instances, and this requires at least IE9. I suggest adding a polyfill if you really really want to keep IE8 compatibility, but I recommend dropping IE8 support as the current market share of IE8 is only 0.26% and dropping IE8 support would allow for the removal of more cruft.

Notes

The filesize of bloodhound.js increased from 33.8 KB to 52 KB and the filesize of bloodhound.min.js increased from 13.3 KB to 18.8 KB, as a consequence of replacing $.deferred, $.ajax, $.noop, $.param and other methods with equivalent implementations that don't rely on jQuery. However, there may still be some baby fat in these methods that could be trimmed (for example, unnecessary method & properties could be removed from Deferred). Nevertheless, for the time being I believe adding 5KB to bloodhound.min.js is a sacrifice worth being able to use Bloodhound without jQuery in all supported browsers.

I would recommend replacing Deferred with an implementation based on the native Promise object at some point in the future, but I don't think it's a good idea to do this as long as you plan on supporting any version of IE, since IE is lacking native Promise support and you'd need a polyfill anyway. So, at the moment, I don't believe replacing Deferred with a Promises/A+ compliant implementation is really worth the effort.

@jslegers
Copy link
Author

jslegers commented Oct 9, 2017

Update

  • I replaced this with _ in common/utils
  • I removed all occurances of Array.isArray with _.isArray and reïmplemented that as function(obj) { return Object.prototype.toString.call(obj) === '[object Array]'; }
  • I fixed an issue with the module format of Deferred, which resulted in Deferred not being recognized everywhere.

As far as I can tell, the code is now compatible again with IE8 and up.

@Grummfy
Copy link

Grummfy commented May 7, 2018

any idea when this will be merged?

@jlbooker
Copy link
Contributor

Any @corejavascript/collaborators have time to review this? I've changed employers and no longer use this project on a daily basis, so I can't justify taking on-the-clock time for maintenance like I used to.

@jcrben
Copy link
Contributor

jcrben commented May 10, 2018

I'm around, but don't have time either.

I think the first priority should be getting the integration tests working again.

@Grummfy @jslegers interested in becoming maintainers?

@mhluska
Copy link

mhluska commented Nov 20, 2018

@jslegers Nice work. One question: why doesn't this PR remove jquery from package.json?

@jslegers
Copy link
Author

jslegers commented Nov 20, 2018

@mhluska :

Nice work.

Thanks!

One question: why doesn't this PR remove jquery from package.json?

After my refactoring, Bloodhound no longer has jQuery as a dependency. However, Typeahead still does.

So, if you can to use Bloodhound without Typeahead, you no longer need jQuery.

If you want to use Typeahead, you'll still need jQuery... for the time being.


@jcrben

@Grummfy @jslegers interested in becoming maintainers?

If I had more time...

@mhluska
Copy link

mhluska commented Nov 21, 2018

@jslegers is there any chance to publish this as a standalone NPM package? I would love to use this in my project without committing the dist.min.js file to my repo (and without having jquery in my dependency tree). I'm imagining just bloodhound-without-jquery as an NPM package.

@jslegers
Copy link
Author

@mhluska

If I ever get some time for it, I sure would consider giving it a try.

In the meantime, @ anyone reading this : feel free to fork my fork and create your own standalone Bloodhound repo based on it.

@simplenotezy
Copy link

Damn I wish we could implement this sooner rather than later.

Perhaps leverage axios to handle the requests..

@simplenotezy
Copy link

@jslegers is there a reason why you didn't removed jQuery from typeahead? or was it simply lack of time?

@jslegers
Copy link
Author

jslegers commented Feb 5, 2020

@jslegers is there a reason why you didn't removed jQuery from typeahead? or was it simply lack of time?

There's several reasons I only removed jQuery from Bloodhound and not from Typeahead :

  1. Typeahead is a lot more tightly coupled with jQuery than Bloodhound. This makes it harder to de-couple & would require API changes.

  2. The project I was working on only required Typeahead

  3. I did not have the time to experiment with trying to de-couple jQuery from Typeahead, neither at my employer's nor after hours.

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.

6 participants