Abort previous pending XHR when navigating #114

Merged
robinnorth merged 4 commits from fix/abort-pending-xhr into master 2018-01-24 18:54:34 -05:00
Showing only changes of commit 137322543c - Show all commits

View File

@@ -23,7 +23,9 @@ tape("test aborting xhr request", function(t) {
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.
}) })
t.test("- pending request is aborted", function(t) { t.test("- pending request is aborted", function(t) {
var r = requestCacheBust("https://httpbin.org/delay/10", {}, function() {}) var r = requestCacheBust("https://httpbin.org/delay/10", {}, function() {
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.
t.fail("xhr was not aborted")
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.
})
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.
t.equal(r.readyState, 1, "xhr readyState is '1' (SENT)") t.equal(r.readyState, 1, "xhr readyState is '1' (SENT)")
abortRequest(r) abortRequest(r)
t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)") t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
BehindTheMath commented 2018-01-24 09:30:37 -05:00 (Migrated from github.com)
Review

Is this callback ever called? Do you want to add a test that fails if it is?

Is this callback ever called? Do you want to add a test that fails if it is?
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:44:01 -05:00 (Migrated from github.com)
Review

Good idea, will do that.

Good idea, will do that.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
robinnorth commented 2018-01-24 09:50:26 -05:00 (Migrated from github.com)
Review

Actually, that callback is never called, because if abortRequest(r) doesn't abort the request, the next assert (t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)")) immediately fails, so I don't think we do need to add anything else in.

Actually, that callback is never called, because if `abortRequest(r)` doesn't abort the request, the next assert (`t.equal(r.readyState, 0, "xhr readyState is '0' (ABORTED)"))` immediately fails, so I don't think we do need to add anything else in.
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
BehindTheMath commented 2018-01-24 17:40:31 -05:00 (Migrated from github.com)
Review

Tape has a t.fail() function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?

Tape has a [`t.fail()`](https://github.com/substack/tape#tfailmsg) function. I think adding that will make it more apparent that the callback is not supposed to be called. What do you think?
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.
robinnorth commented 2018-01-24 18:18:10 -05:00 (Migrated from github.com)
Review

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback shouldn't fire, will add that back in.

I did add that and then remove it again because the callback would never fire, but I take the point about making it explicit that the callback *shouldn't* fire, will add that back in.