Skip to content

Don't set pjax.state unless options.push or options.replace is true #312

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Don't set pjax.state unless options.push or options.replace is true #312

wants to merge 1 commit into from

Conversation

mattyoho
Copy link

This PR is part proposal but largely also a request for feedback.

Our premise is this: if the pjax call has both push and replace set to false, the call shouldn't swap out the pjax.state. This is a degenerate use of pjax but it's surprising behavior from the standpoint of expecting to fall back toward basic Ajax behavior.

Here's a scenario where we're getting into trouble with the current implementation. We want to change a navigation area in place (for the most part) but pushState a main content area, with interleaved calls between the two.

We have this HTML:

State 1:

<html>
<head>...</head>
<body>
  <div id="navigation">
    <ul id="sub-nav">
      <li><a href="/posts/technical" data-pjax="#navigation">Technical Posts</a></li>
      <li><a href="/posts/hobbies" data-pjax="#navigation">Hobby Posts</a></li>
    </ul>
  </div>
  <div id="main">
    <div class="dashboard">...</div>
  </div>
</body>
</html>

State 2:

<html>
<head>...</head>
<body>
  <div id="navigation">
    <ul id="posts">
      <li><a href="/posts/technical/1" data-pjax="#main">Post 1</a></li>
      <li><a href="/posts/technical/2" data-pjax="#main">Post 2</a></li>
    </ul>
  </div>
  <div id="main">
    <div class="post">...</div>
  </div>
</body>
</html>

We have this pjax setup:

$(document).pjax('a[data-pjax=#main]', '#main')
$(document).pjax('a[data-pjax=#navigation]', '#navigation', { push: false })

We have this sequence of events:

  1. Initial page load, with categories of posts in the navigation area and dashboard content in the main area.
  2. Pjax load (without push or replace) a list of posts into the navigation area.
  3. Pjax load (with push state) a post into the main area.
  4. Press the back button.

The result is that dashboard content shows in the navigation area, leaving the main area showing the previously loaded post. This happens because the pjax load of the navigation content sets pjax.state despite not caching the navigation content because push and replace are false.

This PR only sets pjax.state if either options.push or options.replace is true. We've run the existing tests and they all pass, but haven't added new tests for the above scenario.

It's certainly possible that it's more appropriate to use normal Ajax for situations where you would specify both push and replace as false, but it seems if these proposed changes don't introduce breakage then they would remove some surprise from a degenerate use case for Pjax.

@mattyoho
Don't replace last state unless push or replace
a25dd78 
If the pjax call has both push and replace set to false, the call should
act like a plain ajax call and not swap out the pjax.state.
@mattyoho
Copy link
Author

Whoops, we've prematurely sent our PR. 😀 We'll update the PR body and ping again when it's coherent, heh.

@mattyoho
Copy link
Author

Alright, we've updated the PR body with a more fleshed out explanation of our problem and the motivation for the attached changes. Thoughts?

This PR is largely a request for feedback with example in tow. Thanks! /cc @josh

@josh
Copy link
Contributor

josh commented Aug 16, 2013

Thanks for the write up. I still may need some more clarification.

I'd first say that public usage of setting push: false is basically undefined. The code path exists so the popstate handler can make a replacement request without modifying the browser history state.

In a broader model scope, I think this PR generally makes sense. However I think it introduces a $.pjax.state inconsistency bug on popstate.

The idea behind $.pjax.state is that is should always reflect the current browser history state object. Theres actually a real api for this, history.state. However, its not correctly and implemented in all browsers. So $.pjax.state is sorta a polyfill for it.

So my concern is that after a push: false pjax call is invoked from

https://github.com/mattyoho/jquery-pjax/blob/no-push-no-replace-no-state/jquery.pjax.js#L446

$.pjax.state will be now inconsistent with the newly pop'd event state.

So I think we can make your use case work, but we need to handle reseting $.pjax.state elsewhere in the popstate handler.

@mislav mislav mentioned this pull request Sep 17, 2014
Closed
@AlexHill
Copy link
Contributor

For reference, this is also raised in #396. The changes in that PR are essentially the same as in this.

@mattyoho mattyoho closed this Dec 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattyoho @josh @AlexHill