Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Progress events #69

Closed
cibernox opened this issue Apr 20, 2015 · 8 comments
Closed

Progress events #69

cibernox opened this issue Apr 20, 2015 · 8 comments

Comments

@cibernox
Copy link
Contributor

According to the XHR2 specification (https://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#make-progress-notifications):

while the download is progressing, queue a task to fire a progress event named progress about every 50ms or for every byte received, whichever is least frequent.

How do you feel about pretender triggering that event each 50ms by default? If nobody is listening, it's ok. Alternatively you can trigger only if there is an "onprogress" handler attached to the relevan object.

@cibernox
Copy link
Contributor Author

Any feedback on this?

@trek
Copy link
Member

trek commented Apr 24, 2015

Still haven't looked. You could speed up the process by providing more details about the pros/cons of this change.

@cibernox
Copy link
Contributor Author

FakeXMLHttpRequest now can send onprogress events. According to the XHR spec, on progress events are triggered each 50ms during downloads and uploads

Usually you don't care about progress event, but sometimes I do. I was thinking in check if the request object has an onprogress event handler attached. If it does means that the user cares about them, and therefore I pretender should fire them regularly.

p.e. in a request with 200ms of delay the event, it would be fired each 50ms. Never before the loadStart and never after the load event.

@cibernox
Copy link
Contributor Author

Sorry for the insistence, but I'm just waiting some green light to start implementing something.

@trek
Copy link
Member

trek commented Apr 29, 2015

I'd still like to hear the pros/cons. I think you've accurately described the situation, but haven't touched on how it might affect people using the library's current behavior.

@cibernox
Copy link
Contributor Author

I don't think that there is any cons. It's a new feature that should't affect in any form to current users.

The options are
a) Fire the events only if there is a listener for them.
b) Fire the events always.

Intuitively I think that option b) might be simpler to implement and triggering events is cheap enough to not be a concerned about wasting cycles. But in any case it's an implementation detail, the final result for the developer listening for progress events will be identical.

@trek
Copy link
Member

trek commented May 6, 2015

How do you feel about pretender triggering that event each 50ms by default?

Wouldn't we keep triggering events every 50ms, even after a test has finished running, whether you were using them or not? That seems like a high likelihood of ending up with test leaks?

@cibernox
Copy link
Contributor Author

cibernox commented May 6, 2015

@trek see #73 for the implementation details. The change is fairly minimal and I don't see any posibility of leaking since the next progress event is scheduled only if the request is still ongoing.

@cibernox cibernox closed this as completed Aug 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants