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

feat($http): upload and download event #7995

Closed
wants to merge 2 commits into from

Conversation

b1rdex
Copy link

@b1rdex b1rdex commented Jun 26, 2014

As a solution for #1934 and opposite for #3606

You can add progressCallback using config.progress.
Working for all kind of requests — you can even track progress of delete requests.

Pros:

  1. No compability breaking — all is working as before
  2. Better, cleaner and simplier way then q.notify

Cons:
1. No specs and tests here, it's just working

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7995)

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!

@b1rdex
Copy link
Author

b1rdex commented Jun 26, 2014

Based on @benjamingr thoughts in #1934 (comment)

@caitp
Copy link
Contributor

caitp commented Jun 26, 2014

Cons:

  1. No specs and tests here, it's just working

This is a con, we're going to need tests if this is ever going to land.

@@ -68,6 +68,20 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
}
});

if (typeof progressCallback === "function") {
if ("addEventListener" in xhr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

addEventListener is supported in every browser that supports progress events (so the else is not really necessary) --- alternatively, could always just use the else instead.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@b1rdex
Copy link
Author

b1rdex commented Jun 26, 2014

@caitp added some tests. I dunno how to test $http correctly, so added tests for httpBackend for now.

@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@b1rdex b1rdex added cla: no and removed cla: yes labels Jun 26, 2014
@Narretz Narretz added this to the Backlog milestone Jul 2, 2014
@b1rdex
Copy link
Author

b1rdex commented Jul 8, 2014

@caitp rebased. Any news/recomendations?

@swlasse
Copy link

swlasse commented Jul 16, 2014

Just want to follow up - what is the status on this PR?

@b1rdex
Copy link
Author

b1rdex commented Jul 16, 2014

@caitp @swlasse rebased again. Using this in production since made PR, no bugs detected.

@caitp
Copy link
Contributor

caitp commented Jul 16, 2014

I guess we can't really do any better than those tests in karma... @juliemr / @vojtajina how about another flaky e2e test for making requests to the docs server? In the cards or no?

I personally like the feature, so I'd like to get this in in some form, but maybe we need to clean this up in some fashion.

@b1rdex
Copy link
Author

b1rdex commented Sep 8, 2014

@caitp any news on this?

@caitp
Copy link
Contributor

caitp commented Sep 8, 2014

Oh, I guess I should have mentioned this in the meeting --- i left this tab open so that I'd remember, but I got tied up with webkit issues =)

@IgorMinar what do you think about landing this in one of the release candidates? I still feel like a real test case is needed, but maybe it's not in the cards due to flakiness

@b1rdex b1rdex force-pushed the features/xhr-progress-event branch from 86fcb95 to 30db360 Compare September 9, 2014 01:24
@b1rdex
Copy link
Author

b1rdex commented Sep 9, 2014

@caitp @IgorMinar rebased on top of fresh master.

@caitp
Copy link
Contributor

caitp commented Sep 9, 2014

all your files have become executable =) Can we keep it 100644 please --- also, the httpBackend tests are okay, but it doesn't really tell us much about how the feature works, which is why I was saying another e2e test would be good.

The problem with e2e tests is... flakiness, so unfortunately they are a bit problematic. But maybe it's worth it in this case.

@b1rdex b1rdex force-pushed the features/xhr-progress-event branch from 30db360 to 2965b18 Compare September 9, 2014 01:32
@b1rdex
Copy link
Author

b1rdex commented Sep 9, 2014

@caitp fixed file modes. I don't know how to test this much as I mentioned before.

@caitp
Copy link
Contributor

caitp commented Sep 9, 2014

yep don't worry about it, if we can do an E2E test we can probably add it during merging, but I think we probably won't right away due to the problematic-ness

@jimmywarting
Copy link
Contributor

I have a degration suggestion.
I don't care so much about ie9. But maybe the least we can do is calling the handler with a custom event when upload starts and end? ...same for download start and end

This would be possible with listening for the stagechange event

Value State Description trigger degration
0 UNSENT open() has not been called yet.
1 OPENED send() has not been called yet.
2 HEADERS_RECEIVED send() has been called, and headers and status are available. trigger 1 event with {lengthComputable: hasContentLength === true, loaded: 0, total: content-length, target: xhr}
3 LOADING Downloading; responseText holds partial data. send 1 progress event on xhr with the {loaded: content-length} send another with {target: xhr.upload} with lengthComputable, loaded = 0 and total = content-lengt if computable
4 DONE The operation is complete. send a event on xhr.upload and emulate the best you can to native {loaded: response size}

This would be useful if you are converting a file to another format were it would take long to upload a big file and download a big file in same request, with slow bandwidth You would then know if it has started / if it has uploaded / if it's downloading and when its done. You could tell if it is half way there

If this is no good idea. then we should maybe make a option to get hold of the stagechange event and do it ourself. alternative expose the xhr as many already have requested.

@b1rdex b1rdex force-pushed the features/xhr-progress-event branch from 2965b18 to b46f35e Compare November 27, 2014 04:39
@googlebot
Copy link

CLAs look good, thanks!

@pkozlowski-opensource
Copy link
Member

@b1rdex I think that this PR is a good one to kick off the discussion but it is not in the state where it could be merged. If you (or anyone else) is interested in taking this work further I've left some comments in the original issue.

This feature could potentially land in 1.4 as I don't think we need huge amount of code to be written - it is just that we need to figure out number of issues I've raised in my comments.

@b1rdex
Copy link
Author

b1rdex commented Dec 31, 2014

About ie: only one idea for fallback here - emit progress 0 and 100%

About digest cycle: I think it shouldn't trigger digest itself. And no
debounces should be provided. End coder knows better what and how this
should work.

About download vs upload: it's mostly not real world use case if you track
progress of upload and then progress of download for same request.
1 янв. 2015 г. 2:26 пользователь "Pawel Kozlowski" notifications@github.com
написал:

@b1rdex https://github.com/b1rdex I think that this PR is a good one to
kick off the discussion but it is not in the state where it could be
merged. If you (or anyone else) is interested in taking this work further
I've left some comments in the original issue
#1934 (comment).

This feature could potentially land in 1.4 as I don't think we need huge
amount of code to be written - it is just that we need to figure out number
of issues I've raised in my comments.


Reply to this email directly or view it on GitHub
#7995 (comment).

@b1rdex b1rdex force-pushed the features/xhr-progress-event branch 2 times, most recently from eded36c to ed51114 Compare January 23, 2015 05:27
@b1rdex b1rdex force-pushed the features/xhr-progress-event branch from ed51114 to fde8f67 Compare January 26, 2015 03:36
@b1rdex b1rdex closed this Jun 19, 2015
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