-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add XMLHttpRequestUpload support #26
Conversation
snstanton
commented
May 18, 2017
- factor out MockEventTarget base class
- add upload property to MockXMLHttpRequest
- add uploadProgress() method to MockRequest
- update readme to reflect new api
- fix readme to describe .progress() on the proper object
* factor out MockEventTarget base class * add upload property to MockXMLHttpRequest * add uploadProgress() method to MockRequest * update readme to reflect new api * fix readme to describe .progress() on the proper object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to contribute a PR! Please review the few minor comments.
* @param {Object} eventDetails | ||
* @returns {MockEventTarget} | ||
*/ | ||
MockEventTarget.prototype.trigger = function(event, eventDetails) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be dispatchEvent
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trigger seems clear enough
@@ -171,6 +165,8 @@ MockXMLHttpRequest.prototype.send = function(data) { | |||
}, typeof(timeout) === 'number' ? timeout : self.timeout+1); | |||
|
|||
} else { | |||
//trigger a load event to indicate the data has been sent | |||
self.upload.trigger('load'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onloadstart
too? https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/upload
we should probably dispatch the other events e.g. onloadend
at some stage too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly so. I didn't need any of the other events to get my tests working, but having a full lifecycle would probably be useful.
@@ -134,6 +134,14 @@ Get the request headers. | |||
|
|||
Get the request body. | |||
|
|||
#### .progress(loaded : number, total : number, lengthComputable : bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the fix here. do you think this should have been implemented on the response interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about
req.progress(...) //dispatches xhr.upload.onprogress
res.progress(...) //dispatches xhr.onprogress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion. I like the symmetry of having the upload related events on the request and the download related events on the response.
I've merged this into v2 (WIP). |
@jameslnewell any chance we could get these changes in a release, so we can use |
@joscha I've fixed the last few red tests and released a preview release of v2 ( Thanks for your patience. |
Thanks @jameslnewell |
@jameslnewell can you publish with the lib folder for the preview build? Currently the "lib" folder doesn't exist in the npm package that is published at |