iLoveElysia/pjax
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 9 Wiki Activity

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
Conversation 22 Commits 4 Files Changed 8 +95 -8
robinnorth commented 2018-01-23 10:13:17 -05:00 (Migrated from github.com)
Copy Link

This PR adapts the basic concept from https://github.com/defunkt/jquery-pjax/pull/395/files.

Tests included, plus an extra example page to make it easy to test in-browser, using Chrome Dev Tool's network throttling functionality or similar.

Fixes #9.

This PR adapts the basic concept from https://github.com/defunkt/jquery-pjax/pull/395/files. Tests included, plus an extra example page to make it easy to test in-browser, using Chrome Dev Tool's network throttling functionality or similar. Fixes #9.
BehindTheMath commented 2018-01-23 10:34:31 -05:00 (Migrated from github.com)
Copy Link

I believe this will fix #27 as well.

I believe [this](https://github.com/MoOx/pjax/pull/114/files#diff-168726dbe96b3ce427e7fedce31bb0bcR159) will fix #27 as well.
robinnorth commented 2018-01-23 10:41:08 -05:00 (Migrated from github.com)
Copy Link

A bonus fix, nice! 🎉

A bonus fix, nice! 🎉
MoOx (Migrated from github.com) approved these changes 2018-01-24 03:24:14 -05:00
MoOx (Migrated from github.com) left a comment
Copy Link

Nice work

Nice work
robinnorth commented 2018-01-24 05:40:07 -05:00 (Migrated from github.com)
Copy Link

Thanks @MoOx! @BehindTheMath, are we good to merge?

Thanks @MoOx! @BehindTheMath, are we good to merge?
BehindTheMath commented 2018-01-24 08:46:56 -05:00 (Migrated from github.com)
Copy Link

@robinnorth I made 2 inline comments on noop.js and abort-request.js. Did you see them?

@robinnorth I made 2 inline comments on `noop.js` and `abort-request.js`. Did you see them?
robinnorth commented 2018-01-24 09:18:25 -05:00 (Migrated from github.com)
Copy Link

@BehindTheMath they're not showing up, for some reason. Would you mind adding them again?

@BehindTheMath they're not showing up, for some reason. Would you mind adding them again?
BehindTheMath (Migrated from github.com) reviewed 2018-01-24 09:30:37 -05:00
tests/lib/abort-request.js
BehindTheMath (Migrated from github.com) commented 2018-01-24 09:30:37 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2018-01-24 09:31:08 -05:00
lib/util/noop
BehindTheMath (Migrated from github.com) commented 2018-01-24 09:31:08 -05:00
Copy Link

Is there any reason not to inline this?

Is there any reason not to inline this?
BehindTheMath commented 2018-01-24 09:32:36 -05:00 (Migrated from github.com)
Copy Link

Can you see them now?

Can you see them now?
robinnorth commented 2018-01-24 09:40:58 -05:00 (Migrated from github.com)
Copy Link

Yes, thank you.

Yes, thank you.
robinnorth (Migrated from github.com) reviewed 2018-01-24 09:43:51 -05:00
lib/util/noop
robinnorth (Migrated from github.com) commented 2018-01-24 09:43:51 -05:00
Copy Link

I broke this out as a separate module so that it could be used whereever a noop function is required, which is currently in 2 places in the library... but didn't then update the second usage (in the 'stupidPjax' fallback) 😊

I broke this out as a separate module so that it could be used whereever a noop function is required, which is currently in 2 places in the library... but didn't then update the second usage (in the 'stupidPjax' fallback) 😊
robinnorth (Migrated from github.com) reviewed 2018-01-24 09:44:01 -05:00
tests/lib/abort-request.js
robinnorth (Migrated from github.com) commented 2018-01-24 09:44:01 -05:00
Copy Link

Good idea, will do that.

Good idea, will do that.
robinnorth (Migrated from github.com) reviewed 2018-01-24 09:50:26 -05:00
tests/lib/abort-request.js
robinnorth (Migrated from github.com) commented 2018-01-24 09:50:26 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2018-01-24 17:36:34 -05:00
lib/util/noop
BehindTheMath (Migrated from github.com) commented 2018-01-24 17:36:34 -05:00
Copy Link

Can you rename the file to noop.js?

Can you rename the file to `noop.js`?
BehindTheMath (Migrated from github.com) reviewed 2018-01-24 17:40:31 -05:00
tests/lib/abort-request.js
BehindTheMath (Migrated from github.com) commented 2018-01-24 17:40:31 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2018-01-24 18:16:57 -05:00
lib/util/noop
robinnorth (Migrated from github.com) commented 2018-01-24 18:16:57 -05:00
Copy Link

Yes, nice catch, this is why code reviews are important 😉

Yes, nice catch, this is why code reviews are important 😉
robinnorth (Migrated from github.com) reviewed 2018-01-24 18:18:10 -05:00
tests/lib/abort-request.js
robinnorth (Migrated from github.com) commented 2018-01-24 18:18:10 -05:00
Copy Link

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 (Migrated from github.com) reviewed 2018-01-24 18:25:25 -05:00
index.js
@@ -5,6 +5,8 @@ var forEachEls = require("./lib/foreach-els.js")
var newUid = require("./lib/uniqueid.js")
var noop = require("./lib/util/noop")
BehindTheMath (Migrated from github.com) commented 2018-01-24 18:25:25 -05:00
Copy Link

Can you change this as well to noop.js?

Can you change this as well to `noop.js`?
BehindTheMath (Migrated from github.com) reviewed 2018-01-24 18:25:38 -05:00
lib/abort-request.js
@@ -0,0 +1,8 @@
var noop = require("./util/noop")
BehindTheMath (Migrated from github.com) commented 2018-01-24 18:25:38 -05:00
Copy Link

Can you change this as well to noop.js?

Can you change this as well to `noop.js`?
robinnorth (Migrated from github.com) reviewed 2018-01-24 18:31:24 -05:00
lib/abort-request.js
@@ -0,0 +1,8 @@
var noop = require("./util/noop")
robinnorth (Migrated from github.com) commented 2018-01-24 18:31:24 -05:00
Copy Link

I can, although half the library uses this Node convention of not appending the module file extension to the require call -- we should probably standardise this one way or the other -- something for #97?

I can, although half the library uses this Node convention of not appending the module file extension to the require call -- we should probably standardise this one way or the other -- something for #97?
BehindTheMath (Migrated from github.com) reviewed 2018-01-24 18:53:58 -05:00
lib/abort-request.js
@@ -0,0 +1,8 @@
var noop = require("./util/noop")
BehindTheMath (Migrated from github.com) commented 2018-01-24 18:53:58 -05:00
Copy Link

You're right; I didn't notice that.

I guess leave it for now, and make a comment in #97.

You're right; I didn't notice that. I guess leave it for now, and make a comment in #97.
BehindTheMath (Migrated from github.com) approved these changes 2018-01-24 18:54:21 -05:00
Sign in to join this conversation.
Reviewers
No Reviewers
MoOx
robinnorth
BehindTheMath
Labels
Clear labels
bug
dependencies
Pull requests that update a dependency file
 duplicate
enhancement
invalid
question
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#114
Delete Branch "fix/abort-pending-xhr"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?