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

Added beforeSend hook to http client #4419

Closed
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@robertpitt

robertpitt commented May 20, 2015

This small PR is to help solve the issue caused by the xhr object being locked in the http.call closure.

This PR add's a small callback that's fired just before the xhr.send is called, allowing for the xhr object to be manipulated to allow developers to take full control over request.

Including but not limited to

  • Monitoring the state of the request.
  • Monitoring the progress of the request.
  • Changing the behaviour of the request (e.g. async: false).
  • Performing XHR based async file uploads.
  • Aborting the request (e.g. within a 'progress' event..)

Reason for implementing the hook in this fasion, i.e. named beforeSend and the xhr + options being passed over is to replicate the implementation that jQuery provides:

beforeSend
Type: Function( jqXHR jqXHR, PlainObject settings )
A pre-request callback function that can be used to modify the jqXHR (in jQuery 1.4.x, XMLHTTPRequest) object before it is sent. Use this to set custom headers, etc. The jqXHR and settings objects are passed as arguments. This is an Ajax Event. Returning false in the beforeSend function will cancel the request. As of jQuery 1.5, the beforeSend option will be called regardless of the type of request.

Source: http://api.jquery.com/jquery.ajax/

Thanks

@robertpitt

This comment has been minimized.

robertpitt commented May 20, 2015

Closes: #3243, #3266

robertpitt added some commits May 20, 2015

@@ -173,6 +173,19 @@ HTTP.call = function(method, url, options, callback) {
}
};
// Allow custom control over XHR and abort early.
if(options.beforeSend) {

This comment has been minimized.

@glasser

glasser May 26, 2015

Member

https://github.com/meteor/meteor/wiki/Meteor-Style-Guide: space after if (and a few lines down too)

// Sanity
var beforeSend = _.once(options.beforeSend);
// Possibly change the call so the xhr is the context.

This comment has been minimized.

@glasser

glasser May 26, 2015

Member

I don't understand what this comment means. You're providing null as the context.

I think passing arguments to callbacks as arguments rather than as this is generally clearer (and we're moving away from the other style in Meteor).

This comment has been minimized.

@robertpitt

robertpitt May 26, 2015

This was just a note to self, to possibly discuss weather the context of the callback should be the instance of the XHR over the XHR being an argument, but as you stated, Meteor is moving away from this.* in favour of arguments.

@glasser

This comment has been minimized.

Member

glasser commented May 26, 2015

Thanks for the pull request!

While honestly, I think Meteor would be better off with no http package and just encouraging people to use other, more focused and maintained XHR/http packages (eg, my personal response to the comment "This is currently the only thing preventing me from using Meteor.http instead of other xhr libraries" on #3266 is "well, great, use other xhr libraries!")... the package does exist, so we should probably continue to add feature after feature to make it marginally more useful (but still mostly likely less useful than any http package that comes from a project that's 100% focused on making a good http package rather than it being a small part of Meteor's focus).

Please add documentation of the new option, in the JSDoc at the top of the httpcall_client.js file.

@robertpitt

This comment has been minimized.

robertpitt commented May 26, 2015

Thanks for the reply,

I have removed the comment and documented the option in the HTTP.call JSDoc header.

@peterchoo

This comment has been minimized.

Contributor

peterchoo commented May 27, 2015

👍

@robertpitt

This comment has been minimized.

robertpitt commented Jun 4, 2015

@glasser, This PR is ready to be merged btw.

@glasser

This comment has been minimized.

Member

glasser commented Jun 16, 2015

Thanks, merged!

@robertpitt robertpitt deleted the Centiq:feature/before_send_client branch Jun 16, 2015

@smeijer

This comment has been minimized.

smeijer commented Jun 17, 2015

@glasser, I've noticed you have quoted me. I completely agree with you that one should use an external xhr library if the project makes a lot of use of xhr requests (I'm a big fan of visionmedia/superagent). In my case, I required only a single xhr request in my entire app. For a file upload template only.

So my thought at the time was, if the xhr object could be accessed, to be able to bind some events like progress to it, than it wouldn't be necessary to load yet another library and thereby add another dependency to the project. So my thinking was a lot like what you're writing. I wished to use Meteor.http, simply because it was already there. And I find it (most often) wrong to add multiple libraries to a single project, that share mostly the same purpose. (like two xhr libs, with only a different syntax).

I also understand, and agree with you, that Meteor.http will never get as good as a library that is 100% focused on handling http requests. But I also don't believe that it is expected to. Actually, by providing a way to access the xhr object - by implementing the beforeSend as @robertpitt has done - it is now possible to make wrapper packages.

So, as its now possible to create light weight http packages that build on top of Meteor.http, I don't expect much more "feature requests to make it marginally more useful", but I expect wrapper packages instead.

Anyway, thanks @robertpitt for the PR, and @glasser for the merge 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment