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

feat($http): add $xhrFactory service to enable creation of custom xhr objects #12159

Closed
wants to merge 1 commit into from

Conversation

sjurba
Copy link
Contributor

@sjurba sjurba commented Jun 18, 2015

Extract the createXhr function into a service called $xhrFactory which can easily be decorated to be able to construct non-standard or customised XMLHttpRequest objects. This is required for use of the mozAnon and mozSystem options for FirefoxOS.
See https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest#XMLHttpRequest() for details.

Alternate solution to PR #9319
Closes #2318

Review on Reviewable

* @description
* Factory function used to create XMLHttpRequest objects.
*
* Decorate this service to create your own custom XMLHttpRequest objects.

Choose a reason for hiding this comment

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

I would say: "Replace or decorate ..."

@pkozlowski-opensource
Copy link
Member

I've left a couple of comments regarding decorators usage, but otherwise it LGTM after taking a quick look.

@sjurba
Copy link
Contributor Author

sjurba commented Jun 18, 2015

It's funny, my first thought was to do exactly like you suggested. Then for some reason I decided to only go for the decorator version. Anyways, I've updated the text and the example.

@@ -46,7 +72,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
});
} else {

var xhr = createXhr();
var xhr = createXhr(method, url);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: Why are we passing the method and URL ?

Choose a reason for hiding this comment

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

We could pass more (essentially everything we've got here :-)). We can't really predict what people might need... The other approach would be to expose nothing by default and expand the API as soon as we've got more use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for us, but for the benefit of end users who will override the service. Let's say you want to use a custom object for certain urls only..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think method and url is a good middle ground to start with.

@Narretz
Copy link
Contributor

Narretz commented Jun 19, 2015

Wouldn't it make more sense for this to use the same mechanism as the httpInterceptors and the httpParamSerializers?
With this PR, users are limited to having one custom xhrObject per app, which is used for all requests. If I remember correctly, we have at least two use cases:

  • FirefoxOS needs an object passed to the constructor
  • upload / download progress listeners

Having only one implementation per app might not be the best idea.

@sjurba
Copy link
Contributor Author

sjurba commented Jun 19, 2015

@Narretz you will have one xhrObject pr request. The xhrFactory will be called for each request and should return a unique xhrObject every time. It will also be called with the method and the url as parameters so you could customise it to only add certain features for certain urls.

But still, you can't have a separate implementation of the factory for each request, so this implementation only solves the Firefox issue, it doesn't seem like a good solution to the progress listener case.

@pkozlowski-opensource
Copy link
Member

IMO we shouldn't try to fix progress events here, it is a separate issue. And IMO we shouldn't try to have a factory per-request - this is what we've discussed during a meeting: adding per-request handling will add one more param to the $http's object and this is the very issue we wanted to avoid

@sjurba
Copy link
Contributor Author

sjurba commented Jun 30, 2015

Any update on the status of this PR? Anything else I need to do?

@pkozlowski-opensource
Copy link
Member

This one LGTM, I'm +1 for merging this as-is. @petebacondarwin @caitp @Narretz - any objections?

@Narretz Narretz added this to the 1.4.5 milestone Jul 23, 2015
* @description
* Factory function used to create XMLHttpRequest objects.
*
* Replace or decorate this service to create your own custom XMLHttpRequest objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

This affects $http and $resource requests alike, right? (since resource uses http). We should probably note this here.

@Narretz
Copy link
Contributor

Narretz commented Jul 23, 2015

Still not a big fan of this solution, since it's at odds with the way we do other things for $http. @petebacondarwin to the rescue!

@Narretz Narretz modified the milestones: 1.5.x - migration-facilitation, 1.4.5 Jul 23, 2015
@petebacondarwin
Copy link
Contributor

@Narretz do you have a better suggestion for how we could deal with this?

@Narretz
Copy link
Contributor

Narretz commented Jul 23, 2015

@petebacondarwin I was thinking more about how the paramSerializers are implemented. This would bea) more consistent and b) more flexible. But since I don't know if this per-request flexibility is actually needed, let's just merge it once with switch to 1.5 and then see if somebody needs the flexibility.

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

Successfully merging this pull request may close these issues.

6 participants