Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($http) Add support for custom event hooks (progress, etc) #2725

Closed
wants to merge 2 commits into from
Closed

feat($http) Add support for custom event hooks (progress, etc) #2725

wants to merge 2 commits into from

Conversation

prajwalkman
Copy link

In response to #1934
This is far from complete, I just wanted to open a preliminary implementation so we can discuss implementation details.
I'm neither an expert at Angular, nor in the HTTP protocol itself.

The current implementation of $http doesn't allow us to attach event handlers for anything except success and failure. There are a large number of well supported (and maybe non standard) events for XHR that are very useful, the most popular being the progress events.

This PR implements the feature by extending the config object passed to $http to add a hooks keyword, which maps events to event handler.
Example:

var configObject = {
  method: 'GET',
  url: '/specs.html',
  cache: $templateCache,
  hooks: {
    progress: function(data) {
      return $scope.progress = data;
    }
  }
};
$http(configObject).success(successHandler);

To add event handlers for upload events, nest the key mapping under a upload key:

hooks: {
  upload: {
    progress: uploadProgressHandler;
  }
}

Please suggest/discuss improvements to the syntax and/or implementation errors.
I believe the code in the PR currently works, but I haven't thoroughly tested it.

@prajwalkman
Copy link
Author

@michaeltaranto
Copy link

Thanks for the message sorry its taken me a while to respond. I think this implementation is a little bit too focused in its solution. While my particular complaint was not having the ability to hook onto the progress event, there are other complaints that could all be addressed by having access to the xhr object itself.

It seems more useful to potentially overload the $http method to return the xhr on the promise. It's already overloading it with success and error methods. Adding the xhr to the promise will allow people to abort the request, or monitor the progress, or anything else they may want to do that requires the actual request object.

Otherwise we end up adding hooks and methods for every interaction which is probably not ideal. My 2c

@prajwalkman
Copy link
Author

Reasonably what other interaction would you have with the XHR object? I felt it had more chance of being pulled if it solved a specific problem precisely, rather than resorting to a scatter shot approach.

@IgorMinar , @mhevery Any suggestions on how we should take this forward? I feel this is a fairly basic feature to be missing, considering we're over halfway to v2.

@michaeltaranto
Copy link

The most important reason for having the xhr object I would think is aborting the request. I believe theres another thread somewhere that is interested in being able to do that. That in itself is very important to be able to stop the request.

@imccoy
Copy link

imccoy commented Jun 12, 2013

I'm just an interested party, not someone who actually knows stuff, but I'm curious about the lack of something like the $rootScope.$apply at https://github.com/prajwalkman/angular.js/blob/aa53be547c3d55214a731d68557bf2a802220be2/src/ng/http.js#L931 - I think this pull request won't work as expected without it.

I like the approach of passing a bag of event handlers in, but am curious about the special handling for the .upload object. Are there no other members of the XHR object that can have events put on them?

Have you considered adding some tests to the unit test suite for this functionality?

@prajwalkman
Copy link
Author

@imccoy These hooks work outside the promises. Since your handlers will be in the controller itself, you don't need $rootScope.$apply. I think. I mean, I tested it and it seems to work :/

Anyway, about the unit tests, I would need to modify the HTTP Mock service, and that thing is fairly complex. I'm not even sure where to begin, or if this feature is inherently untestable.

@petebacondarwin
Copy link
Contributor

Now that $q.notify has been implemented, we should close this and focus on a PR that provides access to XHR events, such as progress, via the promise returned from $http.

@benjamingr
Copy link
Contributor

@petebacondarwin I humbly disagree, I think providing a hook is a much better abstraction than shoving an event emitter into the promise implementation.

See the arguments made here #1934 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants