Fixed abort/error/loadend event firing, statusCode on error, exceptions #6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
July 17, 2019 11:34
…HTTP errors fire; now always throw new Error() instead of string
mjwwit
requested changes
Jul 17, 2019
| this.handleError = function(error) { | ||
| this.status = 503; | ||
| this.handleError = function(error, status) { | ||
| this.status = +status || 0; |
There was a problem hiding this comment.
Why is there a + in front of status?
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "xmlhttprequest-ssl", | |||
| "description": "XMLHttpRequest for Node", | |||
| "version": "1.5.5", | |||
| "version": "1.5.6", | |||
There was a problem hiding this comment.
In my opinion your changes will at least warrant a minor version bump. Technically it should even be a major, but I doubt anyone's really relying on the specific error type or 503 status code being returned. If you could change it to be 1.6.0 that'd be awesome!
|
I'll merge the PR and make the changes myself. I'll release version 1.6.0 when I'm done. |
|
Thanks, Michael!
FYI - the + in front of status is to coerce the status to a number in case
somebody passes in a string.
Wes
…On Wed, 31 Jul 2019 at 13:16, Michael de Wit ***@***.***> wrote:
Merged #6 <#6> into
master.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6?email_source=notifications&email_token=AABTIUEJA3GV4XYXWH2ENITQCHCF3A5CNFSM4IER3VSKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSZYHVCA#event-2523953800>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABTIUG2V6HYN5LVP2HXTLLQCHCF3ANCNFSM4IER3VSA>
.
--
Wesley W. Garland
Director, Product Development
PageMail, Inc.
+1 613 542 2787 x 102
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I had to look at this because my application was hanging indefinitely using this particular XHR lib -- I had a domain name spelled wrong, and neither the error() nor load() events were firing, so my Promise was never resolving nor rejecting.
This fix addresses that problem by correcting the firing order (and ability to fire) of load, loadend, abort and error events. It also fixes the mis-fire of abort in the current driverdan 1.8 (original repo) version of this code that re-presented itself when I fixed abort firing.
A few smaller fixes were made while debugging this; in particular, status is supposed to be zero when the error handler fires, but was set to 503. I left it at 503 for the extra-process request stuff since I didn't have time to give that enough thought.
Also, I made exceptions into instanceof Error so that stack traces work (already done in driverdan code), and I threw all the event firing once readyState=4 on the NodeJS event loop. I left the earlier events alone since putting everything on the event loop was causing firing order issues; no readyState transistions were firing until the readyState was 4. I expect this change will let multiple load/error/abort handlers which await other (non-xhr) events to interoperate better with no correctness impact.