jquery-pjax

jquery-pjax copied to clipboard

With this change the address bar only changes once the pjax request has come back successfully. This stops the incorrect HTTP_REFERER from being sent with fallback requests.

Have also added a test for this and re-jigged some of the existing tests to match the new behaviour.

Fixes #319

cc @josh

Nice catch @Pym, fixed.

@defunkt or @josh - any chance you could take a quick look at this PR? I'd like to get it in while it still merges cleanly.

Paging @mislav - would really appreciate a glance over this change! All tests pass with this patch applied to latest master.

Not sure if we want to proceed with this. The changes seem plentiful and not compatible with how pjax was previously designed.

I think that the reason why pjax changes the URL immediately is beause that's what browsers actually do on native requests. Then, when you're on a bad network and your response fails, you can force reload on that new URL.

Your endless redirect loop with HTTP_REFERER is an unfortunate side-effect of your own application's design, I'm afraid.

Thanks for having a look at this! I'll try to make my case.

The changes seem plentiful

I've renamed locationReplace to hardLoad, which creates a lot of noise in the patch. Happy to revert that change if it would help?

I think that the reason why pjax changes the URL immediately is beause that's what browsers actually do on native requests.

Firefox and Chrome don't update the address bar until they get a response, so jquery-pjax with this patch is much closer to native behaviour than without it in these browsers. This mismatch is the original reason #319 was opened.

I notice that Safari does update before receive a response, though. Can't test IE right now, sorry.

Then, when you're on a bad network and your response fails, you can force reload on that new URL.

True, though PJAX will hard load the URL on failure, with the same outcome.

Your endless redirect loop with HTTP_REFERER is an unfortunate side-effect of your own application's design, I'm afraid.

My redirect loop is one example of what can go wrong, but all server-side code that makes use of HTTP_REFERER is affected. Right now, when a request fails and jquery-pjax initiates a hard load, the server will always receive the incorrect value for this header.

Thanks again for your time, really appreciate it.

The current pushState timing is very deliberate. It determines when the page is screen shotted for back button animations.

:-1:

Would you mind elaborating on that a little? The problem isn't clear to me. If you describe what this will break I will add a failing test and see if the patch can be modified to pass it while still fixing the bug.

Testing in IE shows the same native loading behaviour as Chrome and Firefox: the address bar isn't updated until the response is received.

To sum up my position, I feel that

• the HTTP_REFERER issue is a genuine bug, and
• fixing it has the positive side-effect of more closely mimicking native loading behaviour in most browsers.

Please follow the following URL in Chrome or Firefox: http://dobrowserschangetheurlwithoutresponse.com/not/really/

Did you receive a response? Probably not, since I made the site up. Did the browser change the URL? I'm seeing yes.

Hmm, yes, it does. I think it changes when DNS resolution fails, so pretty much instantly.

Try this link with 5s delay added to see what I mean. Safari changes the URL straight away, but Chrome, Firefox and IE don't until the five seconds are up.

Hm, that's interesting. Chrome indeed doesn't change the URL until delay is up.

It's true that DNS resolution above caused it to fail fast. However, just because Chrome and Firefox implement some behavior and not Safari, I'm still not convinced that we should match the implementation details of those browsers.

I've just realized that the HTTP_REFERER for fallback requests bug you've described indeed sounds like broken behavior. Do you think it would work if we explicitly set the referrer header with the old URL for fallback requests? Could you maybe try submitting a PR for that first?

Sorry it's taken a while to follow this up.

Do you think it would work if we explicitly set the referrer header with the old URL for fallback requests?

Should work, but since the fallback page load isn't an XHR I don't think you can explicitly set the header – I think we would have to call history.replaceState with the original URL before calling location.replace.

That said, while testing I've come across a few more issues that are also resolved by delaying pushState. These issues arise when clicking back while a PJAX request is in progress.

Say you're on page n. You click a link to n+1 and then change your mind and click back, before n+1 has loaded. All browsers' native behaviour here is to take you back to page n-1. But with PJAX, there are a few issues:

1. You return to page n instead of n-1.
2. Clicking back didn't cancel the pending XHR, so the response arrives and PJAX takes you to page n+1 anyway.
3. At that point, because replaceState was called in the XHR success handler, clicking back will take you not to n but to n-1. Page n has been removed from your history.

These can all be solved by

• moving both cachePush and pushState into the XHR success handler, and
• aborting any existing XHR at the top of onPjaxPopstate.

I will update this PR with these changes and tests to demonstrate the behaviour. Please let me know if anything needs clarification, or if you'd like any of these changes split into separate pull requests, etc.

On Wed, Jun 4, 2014 at 1:06 PM, Alex Hill [email protected] wrote:

Say you're on page n. You click a link to n+1 and then change your mind and click back, before n+1 has loaded. All browsers' native behaviour here is to take you back to page n-1. But with PJAX, there are a few issues

Yeah, this behavior might be a valid bug of pjax. But I would appreciate if you fixed such things in separate PRs, so we have bugfixes separate from changing of features. Fewer implementation changes and more added tests increases the probabilty for a PR to get merged.

I'm fine for now if the browser Back button returns you from n+1 to n even if you clicked it before n+1 has finished loading. But, I definitely agree xhr should be canceled.

No worries. I've opened a PR just to add an XHR abort to the top of the popstate handler here: #395

And here's a PR fixing the HTTP referer bug without moving pushState: #397

Hi @mislav, I'd like to restart this discussion and hopefully get this change in. I've just rebased against current master and cleaned up a bit. Since you merged #395 this patch has become much clearer.

See the most recent commit for the actual functionality changes.

To summarise, I think the success handler is the best place for cachePush and pushState. This change

• Makes PJAX behaviour mirror browser behaviour in more cases
• Fixes the incorrect HTTP_REFERER bug described above
• Fixes the back/forward behaviour bug described above
• Fixes #451 by calling cachePush after firing pjax:start

Would love to hear your thoughts :)

Alex

I'll review when I have a chance; thanks

Sorry @AlexHill, but your PR does not fix #451 (there's still no event fired before caching in popstate events).

Hi @olemoign, can you confirm you're definitely running my code – no caching issues etc?

Have a look for yourself: the only place cachePush is called is in the AJAX success handler.

Which event are you handling?

Hello @AlexHill. cachePush is not the only function caching the container, cachePop does as well.

So on a popstate event (back/forward), you still can't act on the current container before caching, as no event is fired before the cachePop call.

Right you are, thanks for the clarification. Didn't read closely enough, sorry :)

I've just tried moving some things around in the popstate handler, so that cachePop() is only called after pjax:popstate and pjax:start (if that is called). Test all passing locally, give it a shot.