Skip to content
This repository has been archived by the owner on Feb 5, 2020. It is now read-only.

Commit

Permalink
Use jQuery's standards-compliant promises.
Browse files Browse the repository at this point in the history 
Adds compatibility with jQuery 3.x.
  • Loading branch information
@RubenVerborgh
RubenVerborgh committed Jul 18, 2016
1 parent e447848 commit 4dc3e19
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions lib/browser/Request.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ function createRequest(settings) {
timeout: settings.timeout,
type: settings.method,
headers: _.omit(settings.headers, UNSAFE_REQUEST_HEADERS),
})
});
// Emit the result as a readable response iterator
.success(function () {
jqXHR.then(function () {
var response = AsyncIterator.single(jqXHR.responseText || '');
response.statusCode = jqXHR.status;
response.headers = _.object(RESPONSE_HEADERS, RESPONSE_HEADERS.map(jqXHR.getResponseHeader));
Expand All @@ -51,9 +51,9 @@ function createRequest(settings) {
negotiatedResources[resource] = true;
}
}
})
},
// Emit an error if the request fails
.fail(function () {
function () {
request.emit('error', new Error('Error requesting ' + settings.url));
});
// Aborts the request
Expand Down

12 comments on commit 4dc3e19

@pietercolpaert
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Why exactly do you use jQuery in a browser? Isn't this annoying for Web apps that do not use jQuery?

I had some difficulties as well, but adding follow-redirects to the browserify rules in package.json seemed to do the trick: https://github.com/linkedconnections/client.js/blob/master/lib/http/BrowserHttpFetcher.js

@RubenVerborgh
Copy link
Member Author

Choose a reason for hiding this comment

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

Why exactly do you use jQuery in a browser?

  • I didn't want to write (and mostly, test) a cross-browser xhr implementation.
  • This code used to be in the jQuery widget, which also uses jQuery for other tasks.

This might be sufficient though.
Should I migrate?

Isn't this annoying for Web apps that do not use jQuery?

Perhaps.

adding follow-redirects to the browserify rules in package.json seemed to do the trick: https://github.com/linkedconnections/client.js/blob/master/lib/http/BrowserHttpFetcher.js

  • I was trying to avoid the size overhead of the full http library.
  • Too much overlap between BrowserHttpFetcher and NodeHttpFetcher for my taste (but this is fixable).
  • Does it work with HTTPS?

@mielvds
Copy link
Contributor

Choose a reason for hiding this comment

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

I would indeed migrate away from jQuery if it's only for the ajax call.

@pietercolpaert
Copy link
Member

Choose a reason for hiding this comment

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

@RubenVerborgh does not work with https at this moment, but easy fix

Mind that the latest version of browserify uses stream-http as a HTTP drop-in. The library seems pretty lightweight and checks whether the browser has 'fetch' support, which is a streaming HTTP interface in e.g., chrome and Firefox. It looks in fact like it would not be too much overhead? @RubenVerborgh was the initial reason for not using the standard browserify drop-in?

Furthermore, in this library, you will now also get access to the URI that was requested after redirect. Which is a good thing for creating applications where the metadata can be found by looking up the document's URI as a subject in the list of triples.

@RubenVerborgh
Copy link
Member Author

Choose a reason for hiding this comment

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

The size of stream-http seems considerable for the limited functionality we use. (So is jQuery, I know, but much more likely to be reused.) Will investigate size impact.

@RubenVerborgh
Copy link
Member Author

@RubenVerborgh RubenVerborgh commented on 4dc3e19 Jul 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Using the Node http library in the browser increases the gzipped filesize from 69kB to around 84kB (given that it depends on the Node.js stream library, which we don't need otherwise). An 18% increase in size, that doesn't add any functionality except for jQuery-independence, might be rather large.

Granted, jQuery is around 75kB gzipped, but it comes with useful features. Interestingly, only including the AJAX part of jQuery would lead to a similar increase, so that's always an option.

@pietercolpaert
Copy link
Member

Choose a reason for hiding this comment

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

Interesting analysis! I've opened an issue in stream-http to see whether the owners see possibilities to optimize size: jhiesey/stream-http#45

@ariutta
Copy link

Choose a reason for hiding this comment

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

I've also been looking for a request library that:

  1. uses a single API for all environments
  2. fully supports both Node and the browser (Firefox, Chrome, Safari, IE9+), including
    1. streaming
    2. redirects
    3. HTTP caching
  3. has the smallest possible file size for the browser version

I haven't found it yet.

Two possibly solutions I haven't investigated fully:

  • Fetch looks good, but I haven't tried it
  • somehow just specify the AJAX part of jQuery as a dependency and use polyfills like XMLHttpRequest to make it work in Node

I'll be interested to see which solution ends up being selected here!

@RubenVerborgh
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for letting us know, @ariutta. My experiences (after a much shorter evaluation) were similar and lead us to write the current platform-specific wrapper around existing bits of code.

@ariutta
Copy link

Choose a reason for hiding this comment

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

Looking at it a little more, I'm not sure whether IE9 can really support streaming, because it doesn't support XMLHttpRequest advanced features, including progress events. It might be possible to do something using XDomainRequest, but that's probably a waste of time.

This library is browser-only, but it looks pretty good for cross-browser handling of chunked-transfer encoded responses.

@ariutta
Copy link

Choose a reason for hiding this comment

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

I tried out a test case of patching superagent to allow for using chunked responses in a web worker. It's rough and not well tested, but you can check it out here.

Size 15kB minified, reduced further to 4.6kB when gzipped.

@RubenVerborgh
Copy link
Member Author

@RubenVerborgh RubenVerborgh commented on 4dc3e19 Oct 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Trying the switch to XMLHttpRequest in f323fe9.

Please sign in to comment.