inertiajs / inertia-rails

The Rails adapter for Inertia.js.
https://inertia-rails.dev/
MIT License
574 stars 45 forks source link

Built in error sharing in redirects #50

Closed bknoles closed 3 years ago

bknoles commented 4 years ago

Resolves #47

The above issue and https://github.com/inertiajs/inertiajs.com/pull/31 both deal with validation error handling in Rails.

Background

Initially, as noted in #47, my instinct was to try to handle error states without a redirect, as you would do within a vanilla Rails app. The discussion in https://github.com/inertiajs/inertiajs.com/pull/31 made it clear that real world apps are using the session to store errors across redirects.

I played around in my local environment with trying to implement the non-redirect pattern, which roughly looks like this in the controller:

if contact.save
  redirect_to contacts_path, notice: 'Contact created.'
else
  render inertia: 'Users/new', {
    contact: contact,
    errors: contact.errors,
  }
end

I got really close to making it work, but I couldn't find a satisfying UX for what happens if you submit a form with a stale version of inertia (to trigger assets reloading).

So, it makes sense to move forward with the solution in https://github.com/inertiajs/inertiajs.com/pull/31 as the "official" technique for the Rails library.

This PR copies in the work from @ledermann into the gem, with a few tweaks to make things work with 409 conflict situations.

Notes

@ledermann suggested overwriting redirect_to to improve the API. I was hesitant to override the method globally, so instead I created an inertia_redirect_to method instead, which does the same thing that Georg wrote out.

Dealing with stale versions is tricky. I wanted to do exactly what Georg did, which was delete the errors from the session when they get shared into the Inertia store. Unfortunately, the Inertia sharing runs even when there is an assets conflict resulting in a 409. That makes it impossible for the middleware to forward the errors from the session onto the subsequent responses in a PATCH -> redirect -> GET -> 409 -> re-GET pattern (this is the basic pattern that happens when a user submits a form with a stale Inertia version). I ended up moving the code that deletes the inertia errors into the middleware, instead of the controller. That way the middleware can keep the errors around during a stale version situation. I don't love that the single feature is split into two places, so any suggestions are welcome there!

The final API from the Rails side looks like this:

# things_controller.rb
def new
  thing = Thing.new
  render inertia: 'NewThingComponent`, props: { thing: thing }
end

def create
  thing = Thing.new(thing_params)
  if thing.save
    # a regular redirect_to will work here also
    inertia_redirect_to thing_path(thing)
  else
    inertia_redirect_to new_thing_path, errors: thing.errors
  end
end

If a user tries to create a new thing with bad data, the NewThingComponent will get rendered and receive an errors prop. Thanks to Inertia (pointed out by both @ledermann and @BilalBudhani), the malformed data will still be around in the form.

If the user does the same thing, but with a stale version, the things/new page will hard refresh (due to the assets conflict). The submitted data is lost, but the errors will remain. The user would see a blank form that at least has helpful information like Thing's name cannot be blank! (assuming the NewThingComponent prints out the errors it receives). It would be really cool if the actual model data could persist across the hard refresh, but I think that might be out of the scope of the library. In any case, since this is probably an infrequent event for most applications, my opinion is that this UX is acceptable.

Thanks to @reinink and @ledermann for leading the way on this (and for being patient!)

I would love feedback here, as the middleware and error handling is a bit of a juggling act. There are specs testing it, and I actually spun up a small dummy app to verify the behavior, but I would not be surprised if there's a use case or unintended consequence that I missed.

bknoles commented 4 years ago

@BrandonShar I can't respond directly to your first comment for some reason... but re:

What is the collision risk here? It looks like we're deleting the session errors anyway so why not use that location?

If someone happens to be using session[:errors] for something else in their application, we'd run into a conflict if we try to stuff the inertia errors into that same key. the :inertia_errors key is only used internally. the user never sees it. they only see:

inertia_redirect_to some_path, errors: @thing.errors

and then in the javascript:

const MyComponent = ({ thing, errors }) => {
  return (
    {errors && (
      <p>Thanks inertia_rails for sending me the errors!</p>
    )}
  );
};
dpflucas commented 3 years ago

Hi @reinink @bknoles - I see that this feature was already merged but isn't yet documented: https://inertiajs.com/validation#sharing-errors

Any reason for that? I'm happy to create a PR for that if the issue is simply no one did it already :)

Thanks for the fantastic work, I've been using Inertia with Rails for a while now and I'm loving it!

BrandonShar commented 3 years ago

Hey @dpflucas creating a PR for the docs would be awesome!

I believe the change would need to be made here: https://github.com/inertiajs/inertiajs.com/blob/master/pages/validation.mdx

dpflucas commented 3 years ago

@BrandonShar Added a simple example in https://github.com/inertiajs/inertiajs.com/pull/177 :)