Partial reload guard rails

[bknoles]

One of our team members (hi @mattdb) raised a good point last week about a rough edge with partial reloading. If you don't wrap your props in callables, you run the risk of unnecessarily computing expensive props even though your response will only include the partial reloaded props. This is called out in the docs, but Matt pointed out that this gets tricky if you are sharing inertia data in concerns. An unoptimized prop can pretty easily slip through the cracks.

Say you have a search bar that's present on every page. You can drive it via Inertia, and it's a great use case for partial reloading:

module Searchable
  extend ActiveSupport::Concern

  included do
    inertia_share do
      {
        search: { query: '', results: [] }
      }
    end
  end
end 

class ThingsController < ApplicationController
  include Searchable

  def index
    render inertia: 'Things/Index', props: { things: Thing.all }
  end
end

In this example, Thing.all will run every time the app partial reloads :search. And it's pretty easy for that to slip by if Searchable gets added by a different person than whoever coded up ThingsController#index.

I've really like the helpful exceptions that have been added in #121 , #137 and now in #152 . The PR adds something similar to help with the situation described above.

By default, InertiaRails will now emit a warning if it detects 1) a partial reload 2) with prop(s) that weren't requested with the partial reload and aren't callable.

There's also a new config option raise_on_unoptimized_partial_reloads which, as it says, will be more aggressive and raise an error if that occurs.

A few things I'm still pondering in the design:

• Should the config option be an enum instead of a boolean? Something like :ignore, :warn, :raise
• Maybe it should only raise in development/test? The intent is to be yelled at so you can fix it before it hits prod, but you probably don't want to throw errors at users if Thing.all just slows things down a bit
• Should you be able to configure props to ignore? i.e. { some_prop: 'some string' } isn't worth optimizing.
• Add to docs (if we decide to merge)

[BrandonShar]

Not quite a review yet (I'm very tired after a runDisney weekend 😄 ) but I'm sitting at a computer and thought I'd comment my quick thoughts!

[BrandonShar]

At a glance, I don't think I like this pattern of ignorable warnings.. I think if we go this route I'd rather deprecate here and explicitly raise.

[bknoles]

I see it as analogous to React spitting "hey you probably forgot a key when you iterated over that array!" into your JS console.

That said, I could be convinced that "no half measures!" is a good principle here, or that "Rails devs don't expect warnings the way React devs might".

[bknoles]

Closest analogue I can think of in Rails-land is strong parameters, which log filtered out params by default (IIRC).

Admittedly, that isn't always a great experience, (here's a thread of people wishing it raised by default in dev), but in this scenario we're going from "no warning at all" to "at least the logs will give you a hint".

[BrandonShar]

The complexity in this test is what tipped me off that I should be uncomfortable with the warning method above

[bknoles]

I think I could make this cleaner by warning via Rails.logger instead of going all the way down to Kernel.warn.

The important bit is above though, whether logging in dev/test is a useful interface for warning devs or if it's too easily ignored to be helpful.

[BrandonShar]

Is this based on what the frontend requests?

[bknoles]

Yea, if the frontend requests only :search as part of a partial reload, unrequested_props will have every prop except for :search.

Then below we filter out any props that aren't callable, leaving us with props that were defined as values.

[BrandonShar]

genuine question: Is there a valid use-case for a prop here that is callable, but evaluates every time?

[bknoles]

I'm not sure I understand the question. Props that are callable get called (or skipped) after this method runs.

This method isn't doing anything more fancy than checking to see if there are any unrequested props that were defined as values (and it short circuits unless we're partial reloading). The filtering doesn't mutate anything or get used in the actual computing/rendering.

[bknoles]

Not quite a review yet (I'm very tired after a runDisney weekend 😄 ) but I'm sitting at a computer and thought I'd comment my quick thoughts!

Endorphins are the perfect fuel for code review! 😅

[mattdb]

My thought is that the ideal development experience would be for inertia_rails to handle this complexity automatically. At first consideration through my brain, it feels like that's not possible or at a minimum could require some very hairy metaprogramming, but I'm also only superficially aware of what the internals here are, so perhaps I'm missing something.

Mainly piping up to ask "are we treating the symptom or the disease" here -- but maybe a "cure" isn't realistic?

[mattdb]

Alternately, assuming there's no realistic way to prevent controllers using the existing structure from evaluating code that is defined right in their methods (which would definitely be the expected situation 😄 ), perhaps it's worth thinking about if there is an alternate syntax / structure that would more easily support automatically dealing with this sort of thing, ideally such a structure could even help organize things for the non-trivial use cases with lots of concerns/inertia_shares, etc.

I'm definitely totally spitballing here, but one possibility would be to optionally pass symbols to the inertia render, where the symbols would just be methods to call in order to fetch the relevant data. This feels fairly Rails-y, in the way that a number of Rails' class declaration-type methods use symbols to specify methods (before_action :method, or the if: conditional on validates, etc), though those are typically not called within an instance method, at least the ones that immediately come to my mind. This could get tedious with many props being passed, but probably passing many props from a single controller method should be a smell anyways. It's possible it encourages cleaner-feeling code? Not sure about that one, obviously.

Then, if this feels like a decent enough experience, it could become the default way, or what a generator generates, etc, which would be the way this gets "handled automatically" for new folks without them needing to know/worry about it. There's several suppositions in there, though!

The main point I'm raising is -- is there perhaps a "bigger picture" way to smooth both this experience and other experiences?

[skryukov]

I think the "bigger picture" involves implementing an inertia-aware serialization layer, which can be achieved through a serialization plugin system. I plan to first finish upstreaming the contrib gem generators and preparing the v2 beta branch, and then start working on the serialization. Conceptually, it would work like this:

class UsersController < ApplicationController
  inertia_config serialization_plugin: :alba

  def index = render inertia: User.all
end

class UsersIndexSerializer < ActionSerializer
  attributes :basic, :attributes

  has_many :friends
  
  has_many :notifications, always: true
  
  attribute :optional_attr, optional: true
end

# somewhere in the plugin internals:
serializer = lookup_serializer(...)
serializer.as_json(only: ...) # or other serializer-specific logic that allows partial rendering

[skryukov]

I'd use this in production by providing a custom callback with an APM error notification call, for example. So, maybe we should accept :error, :warn, false, and a callable?

[bknoles]

It would address Brandon's reluctance to relying on warnings, and a callable would make it vary behavior based on environment (i.e. be less aggressive in prod). I like it.

[skryukov]

I'd say it's too much for an error message 😅

[bknoles]

haha fair enough

[skryukov]

This code might be called on every Inertia request, so I prefer optimized code over readability. For example, we can eliminate the second loop and avoid creating an additional array by using:

Suggested change

	!key.in?(partial_keys)
	!key.in?(partial_keys) && !prop.respond_to?(:call)

[bknoles]

I shudder to think of the application with an action with so many props that this would make a difference 😅, but this is easy enough to change.

You guessed correctly that it's for readability.

[bknoles]

@mattdb I have had the same thought (wouldn't it be great if we could automatically skip computing non-requested props in a partial reload!), but I wouldn't want to lose the concise "happy path":

def index
  render inertia: 'SomeComponent', props: { this_is: 'so nice and clean' }
end

So I think defining props-as-values is a constraint we choose. The new docs are much clearer about wrapping things in callables to get the performance benefit, and adding a warning (with options to escalate) seems like a healthy amount of guidance.

[mattdb]

I think the "bigger picture" involves implementing an inertia-aware serialization layer, which can be achieved through a serialization plugin system. I plan to first finish upstreaming the contrib gem generators and preparing the v2 beta branch, and then start working on the serialization.

I'm interested in this idea, and it would save on time spent in serializers (which is often non-trivial!), but if it's the User.all (or whatever database/model code gets called in the controller action), a serializer wouldn't be able to preclude that code from executing, at least as far as I can see.

Oh wait, nvm -- ARel's deferred loading, I forgot about that -- this is definitely an intriguing line of thinking, then!

[bknoles]

Now that I think about it, the library already takes advantage of that, doesn't it?

class UsersController < ApplicationController
  def index
    render inertia: 'SomeComponent', props: {
      users: User.all,
      partially_reloaded_prop: 'hi there',
   }
end

If you partial reload that second prop, InertiaRails won't fetch the User.all records since it serializes everything after non-requested props are filtered out. You do create an unnecessary AR relation, but that seems unlikely to kill performance. The behavior isn't any different than with the proposed serialization plugin.

The real killer for partial reload performance would be anything that either 1) explicitly loads your AR records before they are passed to Inertia instead of leaving them as a relation (Blueprinter.render_as_hash maybe?) or 2) requires heavy computation and doesn't touch AR at all. For those scenarios, wrapping in a callable is just the Inertia Way™.

I think we're in good shape here. Explicit support for callables in the library, clear docs, helpful deferred loading from ActiveRecord, and configurable warnings in dev (once we merge this in).

(This isn't a comment about the serialization plugin idea, which at a glance looks like it could add a nice developer experience option!)

[bknoles]

Closing in favor of #165