Rethinking Shared Data

[bknoles]

Continuing the conversation from #108

A quick summary:

• The combination of "module level" shared data storage, and "controller level" setters for shared data has introduced tricky bugs including shared data leaking across threads and across requests.
• While we've patched these issues, it'd be great to refactor away from global shared state.
• Controller class variables have been suggested in the past.
• I'm wary of using controller class variables, since in production they are also a form of global shared state.
• @PedroAugustoRamalhoDuarte has put together Refactor inertia share to use instance variables #111 , which refactors inertia shared state into instance variables. These are set via before_action callbacks, called by inertia_share.

[bknoles]

Ok, so here are a couple examples of failing tests:

First, #117 demonstrates that class_attribute will leak data in the case where inertia_share is called conditionally. Here's the commit with the failing example.

The situation changes a bit with the refactor to instance variables in #116. You can still get leaky data, but it's more awkward to get there. Here's the commit with the failing test. Because of the refactor to before_actions, you actually have to call the route with conditional sharing twice in order to get the error. The shared data wouldn't ever be present on the first page load in development, so I'd guess this wouldn't make it into a production application.

Both of these are contrived examples that are "incorrect" uses of Inertia. But I think it's reasonable for a developer to expect to share data on a per-action basis, instead of per-controller. Especially since other Rails controller class methods like before_action allow per-action configuration. I think we should support it in the gem.

That said, I don't think we need to solve that in order to approve #111. It replaces global storage with the per-request storage of instance variables. That removes the necessity for a lot of current workarounds without changing the public API at all. As the PR notes, conditional sharing can be a future improvement.

[bknoles]

Here a couple (untested) sketches for API changes that could support conditional rendering. Essentially just passing options to the before_action within inertia_share.

The first one requires a breaking change. Plain data props must be defined in a props: argument. Everything else gets passed onto before_action. It's the same API as the renderer, which is nice, but breaking changes are no fun.

module InertiaRails
  module Controller
    extend ActiveSupport::Concern

    module ClassMethods
      def inertia_share(props: nil, **options, &block)
        before_action(**options) do
          @_inertia_shared_plain_data = @_inertia_shared_plain_data.merge(props) if props
          @_inertia_shared_blocks = @_inertia_shared_blocks + [block] if block_given?
        end
      end
    end
  end
end

class SomeController < ApplicationController
  before_action props: { first_name: 'Brian' }, only: [:show]
  before_action only: [:show] do
    {
      last_name: 'Knoles'
    }
  end
end

And here's one where we configure the before_action via a before_action_options: argument. It's not as elegant, but it wouldn't be a breaking change.

module InertiaRails
  module Controller
    extend ActiveSupport::Concern

    module ClassMethods
      def inertia_share(before_action_options: {}, **props, &block)
        before_action(**before_action_options) do
          @_inertia_shared_plain_data = @_inertia_shared_plain_data.merge(props) if props
          @_inertia_shared_blocks = @_inertia_shared_blocks + [block] if block_given?
        end
      end
    end
  end
end

class SomeController < ApplicationController
  before_action first_name: 'Brian', before_action_options: { only: [:show] }
  before_action before_action_options: { only: [:show] } do
    {
      last_name: 'Knoles'
    }
  end
end

[PedroAugustoRamalhoDuarte]

@bknoles Thanks for the time invested in this refactor.

• The syntax of the first solution is more elegant to me, but I think it not worth the breaking change.
• We can go with the second one, maybe we can think in a smaller or better name for before_action_options, maybe: share_options, run_options

[buhrmi]

Can be closed, right? (one off the list)

[PedroAugustoRamalhoDuarte]

Yes, closed by #111

[bknoles]

I'm leaving this open because I'm still pondering how to do per-action conditional sharing.

@ElMassimo is doing the following in #121 :

class InertiaConditionalSharingController < ApplicationController
  inertia_share normal_shared_prop: 1

  inertia_share do
    {conditionally_shared_show_prop: 1} if action_name == "show"
  end

  # rest of the controller
end

which is a perfectly workable way to do it. I'd still love to have syntax similar to before_action. Something like:

inertia_share {thing: 1}, only: [:show]
inertia_share only: [:index, :show] do
  # stuff goes here
end

The existing method signature complicates it a bit, and it's a nice to have additional feature that would be unique to the Rails adapter, so I haven't actually looked too far into it. Perhaps we could use an upcoming major release to change the rules on the method signature!

[ElMassimo]

per-action conditional sharing

I think the appeal of inertia_share is for things you'll need on every request, like flash or current_user, otherwise it starts to become difficult to understand which props are actually being sent from the frontend.

In that sense, supporting only or except in inertia_share encourages poor design a pattern that I don't like 😄.

[bknoles]

Interesting! The Rails pattern I'm thinking about is DRYing up a DB lookup:

class ThingsController < ApplicationController
  before_action :load_thing, except: [:new, :index, :create]

  # ....

  def load_thing
    @thing = Thing.find(params[:id]
  end
end

And how I imagine this would look for InertiaRails would be:

class ThingsController < ApplicationController
  inertia_share except: [:new, :index, :create] do
    {thing: Thing.find(params[:id])
  end
end

You can do the original Rails pattern already if you are using the inertia instance props. But I actually prefer to explicitly write out the render inertia: method in my actions, so I'm left with thing like calling action_name in an inertia_share block (which feels inelegant to me).

It's probably worth nothing that InertiaRails defining shared data at the controller level is already a fairly significant deviation from the original Laravel adapter, which explicitly defines the data as being app-wide:

I like that we have finer granularity (controller vs. app-wide) available; I would consider per-action granularity just another step further down this path, allowing a different stylistic choice for devs. That said, I can see your point about the potential confusion in an app that has inertia_shares scattered all over the place. Perhaps "use shared data sparingly to avoid confusing Future You" is a good rule of thumb.

[bknoles]

As I think about it, perhaps we should have considered deviating further from the Laravel design; Rails and Laravel just kinda have different patterns as Rails focuses on the "controller" where Laravel focuses on the $request.

A question I'm asking myself: If I could go back, would it have been cleaner to make inertia_share a controller instance method that could just be called in the controller callbacks like before_action? Those callbacks kind of exist to share code across actions/controllers, and that's really what we want to do with shared data isn't it?

[buhrmi]

would it have been cleaner to make inertia_share a controller instance method

inertia_rails 3.0 4.0

[skryukov]

would it have been cleaner to make inertia_share a controller instance method

Hmm, what might that look like?

class ThingsController < ApplicationController
  before_action :load_thing, except: [:new, :index, :create]

  # ....

  def load_thing
    inertia_share thing: Thing.find(params[:id])
  end
end

It's a bit verbose for my taste. And to be honest, I can't quite see why it's better, but maybe I'm missing something?

[buhrmi]

I think what @bknoles means is a version of inertia-rails that never touches any global state or uses any class methods (except for global configuration) and all state and methods live on the controller instance.

It's better in the sense that questions about data leaking across requests don't even come up when everything is instance-bound from the beginning.

[skryukov]

I see. Thread-safety is indeed tricky, but there are many ways to achieve it (and even more ways to get it wrong 😅). Our current approach (thanks to @ElMassimo and @PedroAugustoRamalhoDuarte) using immutable objects in class-level variables is one way. Keeping everything on the controller instance is another valid approach.

Since our implementation is already thread-safe, perhaps we could focus on the interface? That's what I'm aiming for here. So, does #137 address all the requirements from this issue? Or are there still some points we need to tackle?

cc @bknoles

[bknoles]

Yea @buhrmi is right on here. It would have been a "safer" design choice to avoid class variables altogether. And now I've proposed a before-action-like API. In retrospect, making inertia_share an instance method that you (typically) call in plain old before_actions would have satisfied both of those points.

But our current inertia_share implementation is indeed thread safe and stable, and I wouldn't make a major breaking change to it at this point.

I'm excited to dig into #137 @skryukov !

[bknoles]

#137 resolved this!