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

Dropping old IEs #162

Open
wants to merge 10 commits into
base: v3development
Choose a base branch
from
Open

Dropping old IEs #162

wants to merge 10 commits into from

Conversation

naugtur
Copy link
Owner

@naugtur naugtur commented Apr 8, 2017

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.

@naugtur
Copy link
Owner Author

naugtur commented Apr 8, 2017

Forgot to mention, this fixes #46

@naugtur naugtur mentioned this pull request Apr 12, 2017
@naugtur
Copy link
Owner Author

naugtur commented Apr 28, 2017

I made another pass and cleaned things up a bit further.
I'd like to merge it now, requesting someone to second-guess what I did ;)

How do I mention all contributors?

Let's go with some more active contributors.
@TehShrike @kesla @gsf @jmeas @stuartsan @djake @fliptheweb

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make a string and split instead of just creating an array? ["get","put","post","patch","head","delete"].map()

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djake Seeing you both default to array syntax, I think I'll undo this golfing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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) {
;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I recall what made me decide to go with the golfing syntax - it was not causing ASI issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could always toss var methods = ["get","put","post","patch","head","delete"] at the top of the file.

@nicksp
Copy link

nicksp commented Jul 25, 2019

Hey @naugtur Any ETA for v3 release? Some quite needed stuff is in there that we're looking for to inject to the app we're working on 😄

* 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
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.

5 participants