Dropping old IEs #162
Dropping old IEs #162
Conversation
|
Forgot to mention, this fixes #46 |
|
I made another pass and cleaned things up a bit further. How do I mention all contributors? Let's go with some more active contributors. let me know if you're no longer involved and have no interest in mentions |
index.js
Outdated
| createXHR.qsSerialize = null // Define this as a function to support the `qs` option | ||
|
|
||
| forEachArray(["get", "put", "post", "patch", "head", "delete"], function(method) { | ||
| "get,put,post,patch,head,delete".split(",").map(function(method) { |
There was a problem hiding this comment.
Why make a string and split instead of just creating an array? ["get","put","post","patch","head","delete"].map()
There was a problem hiding this comment.
It saves 6 characters :)
I'm generally avoiding using my old golfing tricks in real work, but this one seemed readable.
If it raises eyebrows, contrary to what I assumed, I sure have to change it.
There was a problem hiding this comment.
I think either are probably fine, although I am more used to seeing the array syntax.
If we want to prioritize file size to this extent, then I think there are other low-hanging fruit that would bring even bigger wins; i.e.; an "xhr.modern.js" file that would have zero polyfills (and just use Object.assign); that would save a bit more than 6 characters.
There was a problem hiding this comment.
@jmeas an ES6 implementation would be generally a lot smaller, but then the only reason to use this instead of .fetch would be to get an xhr that you can abort. Not sure if that's enough value proposition to get any adoption.
Support for not-so-old browsers is the key here.
Anyway, xhr.modern or xhr.es6 is a good idea to introduce next, once we get v3 shipped.
There was a problem hiding this comment.
@djake Seeing you both default to array syntax, I think I'll undo this golfing.
There was a problem hiding this comment.
@djake Seeing you both default to array syntax, I think I'll undo this golfing
👍
then the only reason to use this instead of .fetch would be to get an xhr that you can abort.
For the record, this is precisely why I use this lib. Promises provide a subset of functionality compared to regular XHR's, yet are simple to make from this callback-style API, so I'm surprised anyone chooses to use fetch!
| createXHR.qsSerialize = null // Define this as a function to support the `qs` option | ||
|
|
||
| forEachArray(["get", "put", "post", "patch", "head", "delete"], function(method) { | ||
| ; |
There was a problem hiding this comment.
Now I recall what made me decide to go with the golfing syntax - it was not causing ASI issues.
There was a problem hiding this comment.
You could always toss var methods = ["get","put","post","patch","head","delete"] at the top of the file.
|
Hey @naugtur Any ETA for |
* Handle error events correctly The `onerror` and `ontimeout` handler are not calling back with errors but with error events (without any messages). Therefore the `errorFunc` is wrapping them as `Error: [ProgressEvent object]`. Here's my quick proposal on how to fix this. Just pass generic error messages to the `errorFunc`, then there will be no case where the error needs to be constructed there. * fix error messages for tests * remove new keywords and use .bind
Please review.
I changed the tests a lot, let me know if you see anything broken.
This also includes the browser runner update made in a separate branch, but I didn't see the point having a separate PR.