feat: add `inertia_config` to enable controller-specific config #121

ElMassimo · 2024-06-26T01:14:51Z

Description 📖

This pull request is a complete refactor of the implementation in order to introduce controller-specific configuration.

`inertia_config`

A new way for controllers to override the global configuration, and provide dynamic values, such as:

class PostsController < ApplicationController
  inertia_config(
    ssr_enabled: -> { action_name == "index" },
  )
end

Just like inertia_share, configuration is inheritable, and subclasses can override values.

In addition, all configuration options can now be procs (before only version supported this).

Performance 🚀

The implementation of inertia_share no longer relies on before_action, improving performance when calculating shared data at runtime, and reducing object allocations.

As an additional benefit, we no longer run this code unnecessarily for non-Inertia requests.

Notes ✏️

Each commit is a gradual refactoring, and can be read in isolation (and passes all tests). I'd still suggest using Squash and Merge.

We also freeze config and shared data to prevent any mutations, ensuring it's safe for multiple threads to access the configuration from the controller class.

With this alone, the same Rails app could serve different Inertia apps, setting a different version for each one in base controllers.

skryukov · 2024-06-26T06:38:38Z

lib/inertia_rails/middleware.rb

-      def saved_version
-        InertiaRails.version.is_a?(Numeric) ? InertiaRails.version.to_f : InertiaRails.version
+      def coerce_version(version)
+        server_version.is_a?(Numeric) ? version.to_f : version


is that a typo?

Suggested change

server_version.is_a?(Numeric) ? version.to_f : version

version.is_a?(Numeric) ? version.to_f : version

No, that's the original logic.

skryukov · 2024-06-26T06:53:31Z

lib/inertia_rails/configuration.rb

+      default_render: false,
+
+      # Let Rails decide which layout should be used based on the controller configuration.
+      layout: true,


I believe that's a breaking change, since current default is nil.
We can write about that in the changelog, or add a default to Renderer#layout:

def layout # we can also raise a warning here? return true if configuration.layout.nil? configuration.layout end

I made the original change in #86.

The controller method was returning true if there global was nil, so the behavior stays the same.

Just tested, if someone sets nil manually, this PR will brake their code (Rails renders page without layout):

InertiaRails.configure do |config| config.layout = nil end

It's unlikely that users are doing that, given it's the default.

That behavior would also be consistent with using layout: false.

If this is released in a major version, I'd say it's more intuitive if whatever the user configures is not overriden internally.

As an alternative, we can raise if the user sets it to nil, since it's more likely a mistake.

Finally, I'd suggest removing layout in the future, it makes more sense to customize using plain Rails in controllers instead, although I'm not proposing we do this now.

yup, that's a great addition, and since README uses true as a default, a note in the changelog should be enough, I guess 👍

I don't feel strongly about it, we could be strict in backwards compatibility first, and remove it later, just mentioning that I don't think it's as useful a setting after #86.

Thanks for the review 😃

I've re-introduced the layout.nil? ? true : layout check, in order to minimize friction when upgrading. Thanks!

skryukov · 2024-06-26T06:59:26Z

spec/dummy/app/views/layouts/application.html.erb

@@ -1,3 +1,3 @@
-<%= inertia_headers %>
+<%= local_assigns[:inertia_ssr_head] %>


Let's wrap this in inertia_headers helper for backwards compatibility?

Agreed 👍

I would add it with a warning that it will be removed.

These are not headers, it's html for the head, so the name is inaccurate.

In addition, it does not make sense to call this in the controller, as it wouldn't have been set yet.

Also, rename it to `inertia_ssr_head`, which is a better description.

…he same

Shared data is now frozen to prevent Also, match the method names for instance variables. Updated a test that used to call `inertia_share` inside a `before_action` as that seems like something a user should never do.

… data

PedroAugustoRamalhoDuarte

@ElMassimo Excellent PR, very important to make easy to set up Inertia V2. This #131 task could be easily resolved if this PR is merged

PedroAugustoRamalhoDuarte · 2024-10-06T14:07:52Z

spec/inertia/sharing_spec.rb

@@ -54,63 +54,6 @@
    it { is_expected.to eq props.merge({ errors: errors }) }
  end

-  context 'multithreaded inertia share' do


@ElMassimo just for curiosity, why do you remove this test? It was failing or because we do not have this problem anymore by default?

It's not possible to have multi-threading issues anymore, as the objects that can be shared between threads are now frozen (not writable, and thus not susceptible to concurrent writes either).

Thanks for the response!!

Fwiw, i pulled the branch and added these tests back (just out of curiosity), and they do still pass.

These were made unnecessary by #111. At the time, I asked @PedroAugustoRamalhoDuarte to keep the specs so we weren't changing spec + implementation at the same time. I'm good with removing them now that we have evidence that refactor passed the original spec. (kind of like adding a new DB column, migrating data, then removing the old column once everything is verified working in production instead of just renaming the column in one shot)

skryukov · 2024-10-10T18:47:30Z

lib/inertia_rails/controller.rb

-    def inertia_layout
-      layout = ::InertiaRails.layout
+    def inertia_shared_data
+      initial_data = session[:inertia_errors].present? ? {errors: session[:inertia_errors]} : {}


I used to set inertia_share(errors: {}) to mimic Laravel plugin's default, with this change — I always overwrite inertia's errors with my empty hash.

Should we move this to the end of the chain?

def inertia_shared_data self.class._inertia_shared_data.filter_map { |shared_data| if shared_data.respond_to?(:call) instance_exec(&shared_data) else shared_data end }.reduce({}, &:merge).tap do |data| data[:errors] = session[:inertia_errors] if session[:inertia_errors].present? end end

skryukov · 2024-10-10T18:51:57Z

lib/inertia_rails/controller.rb

-        before_action do
-          @_inertia_shared_plain_data = @_inertia_shared_plain_data.merge(hash) if hash
-          @_inertia_shared_blocks = @_inertia_shared_blocks + [block] if block_given?
+      def inertia_share(**attrs, &block)


This is a breaking change, should we continue supporting hashes or at least throw a deprecation at first, wdyt?

inertia_share({foo: 'bar'}) # => throws error

Addressed in #137

skryukov · 2024-10-10T18:55:19Z

lib/inertia_rails/middleware.rb

-
-        InertiaRails.version.is_a?(Numeric) ? inertia_version.to_f : inertia_version
+      def server_version
+        controller.send(:inertia_configuration).version


Controller might be nil (i.e. in case of 404 error), I think this is a valid fix:

Suggested change

controller.send(:inertia_configuration).version

config = controller ? controller.send(:inertia_configuration) : ::InertiaRails.configuration

config.version

bknoles · 2024-10-15T21:13:22Z

Hey @ElMassimo thanks for this PR. I'm digging into it now (at long last)!

Just as a heads up, I resolved a merge conflict in the README and committed it to your branch. Hope that's not stepping on your toes.

ElMassimo added 12 commits June 14, 2024 18:27

chore: simplify condition

2e8fc70

refactor: configuration is now a reusable class instead of a singleton

b5187be

refactor: allow controllers to override the app version

c5624be

With this alone, the same Rails app could serve different Inertia apps, setting a different version for each one in base controllers.

refactor: use configuration from controller for ssr

20e9671

refactor: remove global for inertia ssr head

a153301

refactor: pull shared data out of the controller

967fa4a

feat: add inertia_config to override global config in controllers

4d2468d

refactor: avoid using global variables for shared data

f80603e

refactor: drop InertiaRails.reset!

e4700d0

refactor: allow to introduce different merge strategies in the future

a780760

Merge remote-tracking branch 'origin' into refactor/shared

c0d938a

chore: update tests after merge

005de89

skryukov reviewed Jun 26, 2024

View reviewed changes

ElMassimo added 3 commits June 26, 2024 11:17

refactor: re-introduce inertia_headers helper for backwards compat

187e45f

Also, rename it to `inertia_ssr_head`, which is a better description.

fix: ensure setting config.layout to nil manually still behaves t…

48a0994

…he same

chore: remove duplicate key

f8a6663

ElMassimo changed the title ~~refactor: remove all global state while maintaining backwards compatibility~~ feat: add inertia_config to enable controller-specific config Jun 26, 2024

ElMassimo added 4 commits June 26, 2024 11:55

refactor: cache resolved shared data and config in controller classes

5fb2df2

Shared data is now frozen to prevent Also, match the method names for instance variables. Updated a test that used to call `inertia_share` inside a `before_action` as that seems like something a user should never do.

refactor: avoid exposing to_h in configuration

46626e4

docs: update readme to cover the new configuration options

7f84110

docs: link to configuration options

39710b7

ElMassimo force-pushed the refactor/shared branch from d52201a to 39710b7 Compare June 26, 2024 16:02

ElMassimo added 3 commits June 26, 2024 13:19

docs: reference community-maintained docs.

05b211d

perf: reduce array allocations when controllers don't override shared…

ce859b2

… data

perf: reduce Configuration instances to the bare minimum

2aacfed

PedroAugustoRamalhoDuarte reviewed Oct 6, 2024

View reviewed changes

skryukov mentioned this pull request Oct 10, 2024

Inertia.js v2.0 compatibility #132

Open

11 tasks

skryukov reviewed Oct 10, 2024

View reviewed changes

Merge branch 'master' into refactor/shared

c747bad

bknoles mentioned this pull request Oct 17, 2024

Rethinking Shared Data #115

Open

skryukov mentioned this pull request Oct 18, 2024

before_action like filters for inertia_share #137

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `inertia_config` to enable controller-specific config #121

feat: add `inertia_config` to enable controller-specific config #121

ElMassimo commented Jun 26, 2024 •

edited

Loading

skryukov Jun 26, 2024

ElMassimo Jun 26, 2024

skryukov Jun 26, 2024

ElMassimo Jun 26, 2024

skryukov Jun 26, 2024

ElMassimo Jun 26, 2024 •

edited

Loading

skryukov Jun 26, 2024

ElMassimo Jun 26, 2024

ElMassimo Jun 26, 2024

skryukov Jun 26, 2024

ElMassimo Jun 26, 2024

PedroAugustoRamalhoDuarte left a comment

PedroAugustoRamalhoDuarte Oct 6, 2024

ElMassimo Oct 6, 2024

PedroAugustoRamalhoDuarte Oct 7, 2024

bknoles Oct 17, 2024 •

edited

Loading

skryukov Oct 10, 2024 •

edited

Loading

skryukov Oct 10, 2024

skryukov Oct 18, 2024

skryukov Oct 10, 2024

bknoles commented Oct 15, 2024

	server_version.is_a?(Numeric) ? version.to_f : version
	version.is_a?(Numeric) ? version.to_f : version

		@@ -1,3 +1,3 @@
		<%= inertia_headers %>
		<%= local_assigns[:inertia_ssr_head] %>

	controller.send(:inertia_configuration).version
	config = controller ? controller.send(:inertia_configuration) : ::InertiaRails.configuration
	config.version

feat: add inertia_config to enable controller-specific config #121

Are you sure you want to change the base?

feat: add inertia_config to enable controller-specific config #121

Conversation

ElMassimo commented Jun 26, 2024 • edited Loading

Description 📖

inertia_config

Performance 🚀

Notes ✏️

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElMassimo Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PedroAugustoRamalhoDuarte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bknoles Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

skryukov Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bknoles commented Oct 15, 2024

feat: add `inertia_config` to enable controller-specific config #121

feat: add `inertia_config` to enable controller-specific config #121

ElMassimo commented Jun 26, 2024 •

edited

Loading

`inertia_config`

ElMassimo Jun 26, 2024 •

edited

Loading

bknoles Oct 17, 2024 •

edited

Loading

skryukov Oct 10, 2024 •

edited

Loading