Skip to content
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.

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

Allow PJAX to be used on form submission #80

Closed
wants to merge 1 commit into from

Conversation

tmeasday
Copy link

Not as complicated a commit as it looks. Does a couple things

  1. Allows you to bind to the submit event on a form, treats it very similarly to the click event on a link.
  2. Splits the success callback into two methods, the page replacing part + the state changing part so it:
  3. Deals gracefully with 422 status codes, which aren't errors, but require the page to be changed.

Also there's a very minor change, which is to wrap the result from the server in a <div> before trying to extract the fragment, in case all the server returns is the fragment.

Let me know if there's anything I can do to simplify it.

@josh
Copy link
Contributor

josh commented Jan 31, 2012

I don't think this is a good direction for pjax.

PJAXing simple get links is safe, will degrade nicely and works well with the back button.

Form submissions aren't always going to degrade and are going to cause problems with post forms.

@tmeasday
Copy link
Author

Sure thing Josh,

Up to you. I guess it works pretty well for us, but we only use it for very simple CRUD forms with well defined behaviours.

Certainly I can see where it might be a problem with POSTs especially if they go wrong and you want to fall back to non-AJAX (ie. needing to repeat an non-idempotent request).

Any thoughts on a what a good way to integrate our AJAX forms into the PJAX fold?

Right now (with the above changes) we have a nice system where a PUT on (say) a user will result in two history entries, users/ID/edit and then users/ID. So the back button works nicely and everything integrates well. If PJAX doesn't support forms, then is our only alternative to roll our own history management for such requests?

@josh
Copy link
Contributor

josh commented Jan 31, 2012

Pjax doesn't do anything special with pushState that you can't do on your own. If you define your own behavior for pushState forms it will play nice. We actually don't use pjax everywhere on github.com. In a few places like the tree slider, we work with pushState directly to get better control over animation timings.

@tmeasday
Copy link
Author

Alright, cheers. I'll close this then.

@tmeasday tmeasday closed this Jan 31, 2012
@josh
Copy link
Contributor

josh commented Feb 17, 2012

Alright, somewhat interested in aspects of this.

  1. Degradable $.pjax with type: 'POST', reported in Handling of POST requests in unsupported browsers #59
  2. Handle redirects properly. On redirect pjax updates the wrong url #85 its tricky but I have some ideas.
  3. Consider adding submit delegation. I dunno if something that simple will work for most forms. But at least make it simple to do:
$(document).on "submit", "#form", ->
  $.pjax
    type: this.method
    url: this.action
    data: $(this).serializeArray()
    container: "#container"
  false

@tmeasday
Copy link
Author

  1. I think the difficulty is in dealing with errors, both
    • 422 errors, ie. validation problems
      • this PR is how we manage it -- we update the content but the not the history.state. I think this is a valid default but maybe outside of pjax's scope.
    • 500 errors or 'pjax' errors (for instance if there's no #container in the response).
      • for GET requests, pjax deals via re-sending the request "un-ajaxed" (i.e. sets window.location).
      • it's not as easy to do this for non-GETs
      • it may be undesirable to do so, for instance two POSTs != one POST.
  2. The only way to deal with it is to set something server-side right? (even jquery doesn't know about redirects). I've bumped into this one a bunch of times before.
  3. It seems to work ok for rails-ujs and <form data-remote="true">.

@josh
Copy link
Contributor

josh commented Feb 19, 2012

I think we can handle those special case errors better by adding more custom pjax:* events.

Yeah, we need server side hacks. Thinking of proposing a X-PJAX-URL response header. Seems dumb, but it you'd always set it to the url that was requested.

I'm probably going to do something similar like data-pjax-remote for our app, but I don't think its going to move up to pjax core. Chris doesn't want that "magic" stuff here. But it might be nice if $("form[data-pjax-remote]").pjax() activated that for you.

@tmeasday
Copy link
Author

Response headers -- yes, this is easily achieved via an after_filter on ActionController::Base.

data-pjax-remote -- this PR simply binds to .submit on <form>s that have had .pjax() called on them. In our app, we do something along the lines of:

  $('form[data-pjax]').pjax();

Re custom events: This is more troublesome..

  1. So it will behave differently than pjax (w/ GET) does right now for an error? (if I request a URL that returns a 500, pjax simply falls back to window.location + resends, but as you pointed out, it can't do that for POST).
  2. Validation errors: The nice thing about this PR, and pjax in general is that you can take something that works fine synchronously (without pjax), and throw down a .pjax() call, and it behaves the same. If people are required to bind to the pjax:invalid event and roll their own error highlighting (or whatever), then you lose that simplicity.

@josh
Copy link
Contributor

josh commented Feb 21, 2012

Working on POST fallback in #95

@tmeasday
Copy link
Author

Let me know if you need anything..

@josh
Copy link
Contributor

josh commented Feb 22, 2012

@tmeasday How do you think we should handle back and forth? So you submit a form, then click back, then forward. Normally browsers warn you about double submissions. Maybe we need to do something similar. Also, theres a storage limit in pushState, so I don't know if we can even get away with serializing "large" forms.

@tmeasday
Copy link
Author

Yeah, that's a tricky one. Of course you could hack something together where your pushState stores a key that references into some localStorage somewhere that has the form data, but I'm thinking that is waaaay too complicated for pjax.

Or you could just not allow re-submissions, it's often broken for sites anyways, so I don't know if it'd be a big loss.

In case you are interested: we sort of get away with it by just relying on standard REST routes + always doing GET requests for back/forward, so you get the following behaviour (say on User):

  1. Hit the /users/ID/edit page
    • you see users#edit
    • /users/ID/edit is the PJAX entry.
  2. Hit submit. This PUTs to /users/ID, which updates and then redirects to a GET on /users/ID.
    • you see users#show
    • /users/ID is the PJAX entry.

If you hit back now, you'll see users#edit again, if you hit forward from there, pjax will do a GET to /users/ID and you'll see users#show again. Which is, strictly speaking, wrong (and different to the fallback behaviour), but does work quite well.

But it's exceedingly brittle, and won't work for anything non-standard, maybe not even for simple POSTs either. It also behaves a bit weirdly if there are validation errors..

@josh
Copy link
Contributor

josh commented Feb 22, 2012

Yeah, I'm probably over thinking this then. I like that fallback.

Should probably at least fix serializing post data to a query string. Heh, that would not be a good idea for forms.

https://github.com/defunkt/jquery-pjax/blob/master/jquery.pjax.js#L220-222

@tmeasday
Copy link
Author

Ha, yeah right, I didn't even realise that pjax was saving it, and making a GET request with all the form data in it. Of course my #show action is just ignoring all that extra junk.

So the fallback is on popstate to just to make a GET request to the same URL as the original POST request, and tell people not to use pjax + forms/non-GET if that clashes with their routes?

@josh
Copy link
Contributor

josh commented Aug 22, 2012

@tmeasday taking this to the next level with $.pjax.submit 6fc6bd6 thanks for your help thinking through this stuff.

@tim-peterson
Copy link

Hey @josh can you provide some documentation on how to use $.pjax.submit()? I've installed your fork, 6fc6bd6, but i'm still stuck on a very basic error: has no method 'submit'.

does $.pjax.submit() have same options as $.pjax()?

@wkrsz
Copy link

wkrsz commented Jun 29, 2013

This is how I make PJAX display content of 422 error pages:

$(document).on('pjax:error', function(event, xhr, textStatus, errorThrown, options){
  if (xhr.status == 422) {
    options.success(xhr.responseText, status, xhr);
     return false;
  }
 });

https://coderwall.com/p/xfjopa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants