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

feat($http): add XHR events configuration option#11547

Closed
chrisirhc wants to merge 2 commits into from
Closed

feat($http): add XHR events configuration option #11547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b133af
feat($http): add XHR events configuration option
chrisirhc Sep 24, 2014
295d292
docs($http): add description about events config
chrisirhc Jul 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/ng/http.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -694,6 +694,8 @@ function $HttpProvider() {
* - **headers** – `{Object}` – Map of strings or functions which return strings representing
* HTTP headers to send to the server. If the return value of a function is null, the
* header will not be sent. Functions accept a config object as an argument.
* - **events** - `{Object}` - Event listeners to be bound to the XMLHttpRequest object.
* To bind events to the XMLHttpRequest upload object, nest it under the upload key.
* - **xsrfHeaderName** – `{string}` – Name of HTTP header to populate with the XSRF token.
* - **xsrfCookieName** – `{string}` – Name of cookie containing the XSRF token.
* - **transformRequest** –
Expand DownExpand Up@@ -1155,7 +1157,7 @@ function $HttpProvider() {
}

$httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
config.withCredentials, config.responseType);
config.withCredentials, config.responseType, config.events);
}

return promise;
Expand Down
16 changes: 15 additions & 1 deletion src/ng/httpBackend.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -28,7 +28,7 @@ function $HttpBackendProvider() {

function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDocument) {
// TODO(vojta): fix the signature
return function(method, url, post, callback, headers, timeout, withCredentials, responseType) {
return function(method, url, post, callback, headers, timeout, withCredentials, responseType, eventHandlers) {
$browser.$$incOutstandingRequestCount();
url = url || $browser.url();

Expand DownExpand Up@@ -88,6 +88,20 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
xhr.onerror = requestError;
xhr.onabort = requestError;

if (eventHandlers) {
forEach(eventHandlers, function(value, key) {
if (key !== 'upload') {
xhr.addEventListener(key, value);
}
});

if (eventHandlers.upload) {

@gkalpakgkalpakJul 15, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why put listeners inside upload if it's not going to make any difference ?

@petebacondarwinpetebacondarwinJul 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

?

The eventHandlers object has the following structure:

{
someEvent: ...,
someOtherEvent: ...,
upload: {
someUploadEvent: ...,
someOtherUploadEvent: ...
}
}

@gkalpakgkalpakJul 15, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but we end up doing xhr.addEventListener(key, value) for all of them.
I.e. I could write you example as:

{someUploadEvent: ...,someOtherUploadEvent: ...
upload: {someEvent: ...,someOtherEvent: ...,}}// or{someEvent: ...,someOtherEvent: ...,someUploadEvent: ...,someOtherUploadEvent: ...}

and the result would be the same.

So, I am wondering if there is any benefit in separating into two categories. Why not keep it flat ?

@chrisirhcchrisirhcJul 15, 2015

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sorry, this is a typo on my part. This is meant to attach events on xhr.upload. The line 100 should read xhr.upload.addEventListener(key, value);.

I'll be very happy to fix this if there's any interest in the PR.

Specifically, this is because there's progress events for both upload and the xhr object itself.

@gkalpakgkalpakJul 15, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense now; I didn't know about xhr.upload 😕

forEach(eventHandlers.upload, function(value, key) {
xhr.upload.addEventListener(key, value);
});
}
}

if (withCredentials) {
xhr.withCredentials = true;
}
Expand Down
11 changes: 11 additions & 0 deletions src/ngMock/angular-mocks.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1683,6 +1683,17 @@ function MockXhr() {
};

this.abort = angular.noop;

this.$$events = {};
this.addEventListener = function(name, listener) {
if (angular.isUndefined(this.$$events[name])) this.$$events[name] = [];
this.$$events[name].push(listener);
};

this.upload = {
$$events: {},
addEventListener: this.addEventListener
};

@gkalpakgkalpakJul 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we are mocking the addition of event listeners, would it make sense to also mock emitting some event (so that people can test their code once it relies on a callback to be called on event X) ?
Would it be possible in a clean and reliable way without a breaking change ?

@chrisirhcchrisirhcJul 28, 2015

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense and could be implemented like MockWindow's fire method .
However, I don't want to put too many changes in this PR at the moment as it'll affect reviewing, and the chances of it getting merged.

}


Expand Down
13 changes: 13 additions & 0 deletions test/ng/httpBackendSpec.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -214,6 +214,19 @@ describe('$httpBackend', function() {
expect(MockXhr.$$lastInstance.withCredentials).toBe(true);
});

it('should set up event listeners', function() {
var progressFn = function() {};
var uploadProgressFn = function() {};
$backend('GET', '/url', null, callback, {}, null, null, null, {
progress: progressFn,
upload: {
progress: uploadProgressFn
}
});
xhr = MockXhr.$$lastInstance;
expect(xhr.$$events.progress[0]).toBe(progressFn);
expect(xhr.upload.$$events.progress[0]).toBe(uploadProgressFn);
});

describe('responseType', function() {

Expand Down