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

I think what you are describing is only working by accident. Before this change, I believe inertia_share(errors: {}) would have overwritten the errors from the session (exactly as you described). That change moved the error sharing into a block, which gets evaluated after the plain data is computed, allowing your technique to work. I'd call that unintended behavior because presumably this equivalent looking code just wouldn't work:

# This will always overwrite the session errors because this block will be evaluated after the error sharing block from the library inertia_share do {errors: {}} end

IMO, if the application code explicitly sets a prop it should overwrite whatever came out of the library internals, so this PR is actually fixing that.

That said, maybe our default should match the Laravel default and always return something in an errors prop? That seems to be the real thing at issue here. I don't have a strong opinion, but I'd lean towards keeping the default we have since it'd be a potentially breaking change.

I consider that separate from the scope of this PR and won't hold up merging it for this.

(Please let me know if I'm misunderstanding your use case)

Oh, great, I just noticed a breaking change, although it appears to be a fix 😂.

Regarding errors: {} being the default—that's a significant breaking change, and I would suggest adding a compatibility configuration flag for opting into backward compatibility until the next major release. I also think we should first discuss the possibility of adding this behavior to the protocol. But anyway, that's a separate issue 👍

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

lib/inertia_rails/middleware.rb

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.

bknoles · 2024-10-21T05:15:53Z

README.md

+```ruby
+class EventsController < ApplicationController
+  inertia_config(
+    version: "events-#{InertiaRails.configuration.version}",


Local version configuration seems like a bit of a footgun. In this example, won't you trigger a hard refresh every time you visit this controller from a different controller since those versions will always be mismatched?

All the other configuration options make perfect sense to optionally configure per-controller. Maybe we should exclude version (and only version) from being locally configured?

won't you trigger a hard refresh every time you visit this controller from a different controller since those versions will always be mismatched?

You could have different apps, with entirely different Vite bundles, so that would actually make sense and the hard refresh would "make it work". Linking across apps using Inertia navigations isn't "recommended", but it's nice that it would work out of the box.

Having an exception for version seems unnecessary, it's completely optional to use it, and in some cases it's actually useful.

Ah interesting. So, to make sure I understand with a real life example, you might have a "storefront", with one JS bundle, and an "admin interface" with a separate bundle.

And your StorefrontBaseController and AdminBaseController would configure different versions. And in that scenario, if you Inertia-navigate from /storefront to /admin, a hard refresh isn't a problem.

One could create a mess by locally versioning controllers that target the same JS bundle (as I descriebd), but we are Rails developers and we like our knives sharp :)

I'm on board now. Thanks!

lib/inertia_rails/controller.rb

…ge to users explicitly sending a hash into the method

…handling)

bknoles · 2024-10-21T17:13:20Z

Made a couple tweak commits and finally got this thing merged! Thanks so much @ElMassimo ! Lovely code that adds some really nice features.

Thanks also to @skryukov for your sharp eyes to catch a few breaking changes that weren't covered by our test suite. I'm going to follow up with a few specs I created locally to cover those things.

I'll get a release out sometime this week in advance of any Inertia v2.0 work.

bknoles · 2024-10-27T22:05:31Z

Released in 3.3.0!

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

Merged

skryukov reviewed Oct 10, 2024

View reviewed changes

lib/inertia_rails/middleware.rb Outdated Show resolved Hide resolved

Merge branch 'master' into refactor/shared

c747bad

bknoles mentioned this pull request Oct 17, 2024

Rethinking Shared Data #115

Closed

skryukov mentioned this pull request Oct 18, 2024

before_action like filters for inertia_share #137

Merged

bknoles reviewed Oct 21, 2024

View reviewed changes

lib/inertia_rails/controller.rb Outdated Show resolved Hide resolved

bknoles added 2 commits October 21, 2024 12:00

Use original syntax for inertia_share method to avoid a breaking chan…

2f719ec

…ge to users explicitly sending a hash into the method

controller might not exist (such as in the case of Rails default 404 …

c5dc7a4

…handling)

bknoles approved these changes Oct 21, 2024

View reviewed changes

bknoles merged commit fac9eba into inertiajs:master Oct 21, 2024
9 checks passed

ElMassimo deleted the refactor/shared branch October 21, 2024 17:25

ElMassimo mentioned this pull request Oct 21, 2024

refactor: avoid global state and allow controllers to specify configuration locally ElMassimo/inertia-rails#1

Closed

bknoles mentioned this pull request Oct 22, 2024

Add specs from config refactor #139

Merged

bknoles mentioned this pull request Nov 4, 2024

Partial reload guard rails #154

Closed

4 tasks

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

bknoles Oct 20, 2024

skryukov Oct 20, 2024

skryukov Oct 10, 2024

skryukov Oct 18, 2024

bknoles commented Oct 15, 2024

bknoles Oct 21, 2024

ElMassimo Oct 21, 2024

bknoles Oct 21, 2024

bknoles commented Oct 21, 2024

bknoles commented Oct 27, 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] %>

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

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

Choose a reason for hiding this comment

bknoles commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bknoles commented Oct 21, 2024

bknoles commented Oct 27, 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