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

feat($http) XHR progress events #3606

Closed
wants to merge 1 commit into from
Closed

feat($http) XHR progress events #3606

wants to merge 1 commit into from

Conversation

mjc-gh
Copy link

@mjc-gh mjc-gh commented Aug 15, 2013

Should likely add a few more specs for this. Any suggestions? May need to add to MockXHR a bit.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@vojtajina
Copy link
Contributor

@caiotoon please make sure

  • it's rebased on the latest master
  • the commit msg follows the convention, should be feat($http): ...
  • add a test for the notification (send a request and call mockxhr.onprogress)
  • fix the style issues

/**
* Progress callback for $httpBackend
*/
function progress(event){
Copy link
Contributor

Choose a reason for hiding this comment

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

space before { - please fix this everywhere...

@ghost ghost assigned vojtajina Aug 15, 2013
@mjc-gh
Copy link
Author

mjc-gh commented Aug 15, 2013

I can just close this request and reopen a new one that's properly rebased and has the needed updates. Really just wanted to get some input on how to test it. I can add some calls to MockXHR and test it that way. Thanks!

@btford
Copy link
Contributor

btford commented Aug 15, 2013

No need to open a new issue; just amend the commit and force push to your branch.

@mjc-gh
Copy link
Author

mjc-gh commented Aug 15, 2013

Sounds good, I'll try to take care of the changes ASAP.

@vojtajina
Copy link
Contributor

@mikeycgto use MockXhr (from angular-mocks.js) and test that the notify handler is called when using $http. Call MockXhr.$$lastInstance.onprogress...

@caiotoon
Copy link
Contributor

@vojtajina I didn't got what should I rebase, sorry. Wasn't the promise notification already merged? What should I rebase?

@mjc-gh
Copy link
Author

mjc-gh commented Dec 3, 2013

Was able to rebase this commit and fix up the things that needed fixing. Also filled out and submitted the CLA. Sorry for the epic delay!

Add to $http and $httpBackend to open up progress events. Update specs and mocks to handle the new
argument and to test for the $http promise notify method.

Closes #1934
@colinfrei
Copy link
Contributor

I would love to see this be merged - any updates?

@anton000
Copy link

chances of this getting merged before 1.3?

@jbruni
Copy link
Contributor

jbruni commented Mar 14, 2014

I would like to see the feature available soon. Thanks.

@pkozlowski-opensource
Copy link
Member

@mikeycgto / All interested in this PR: based on the discussion in #1934 (staring from here) it seems like the general trend is to remove notifications (at least in the current form) from promises. As such we don't want to solve the problem of progress notifications using promise APIs.

I'm going to close this PR (and similar ones: #3606, #5874, #9258) and focus on the callback-based solution instead (similar to the one proposed in #7995).

We still do appreciate your effort on this issue which seems to be important to many people - so if you care about progress notifications being available in AngularJS please let's have discussion in #7995.

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.

9 participants