From 351ba22632bc573a3c3f7c8e116c4db65d1fca04 Mon Sep 17 00:00:00 2001 From: Brian Knoles Date: Sun, 3 Nov 2024 12:33:26 -0500 Subject: [PATCH 1/7] Specs and Shameless Green crude implementation for warning devs if their partial reloads are needlessly computing extra data --- lib/inertia_rails.rb | 6 ++ lib/inertia_rails/renderer.rb | 8 +++ ..._unoptimized_partial_reloads_controller.rb | 19 +++++ spec/dummy/config/routes.rb | 3 + spec/inertia/partial_reload_spec.rb | 69 +++++++++++++++++++ 5 files changed, 105 insertions(+) create mode 100644 spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb create mode 100644 spec/inertia/partial_reload_spec.rb diff --git a/lib/inertia_rails.rb b/lib/inertia_rails.rb index ef6b0885..934deece 100644 --- a/lib/inertia_rails.rb +++ b/lib/inertia_rails.rb @@ -25,4 +25,10 @@ class Error < StandardError; end def self.deprecator # :nodoc: @deprecator ||= ActiveSupport::Deprecation.new end + + def self.warn(message) + full_message = "[InertiaRails]: WARNING! #{message}" + Kernel.warn full_message if Rails.env.development? || Rails.env.test? + Rails.logger.warn full_message + end end diff --git a/lib/inertia_rails/renderer.rb b/lib/inertia_rails/renderer.rb index ec090c26..e95bfe34 100644 --- a/lib/inertia_rails/renderer.rb +++ b/lib/inertia_rails/renderer.rb @@ -68,6 +68,14 @@ def merge_props(shared_data, props) end def computed_props + unrequested_value_props = props.select do |key, prop| + !prop.respond_to?(:call) && !key.in?(partial_keys) + end + + if rendering_partial_component? && unrequested_value_props.present? + InertiaRails.warn "The #{unrequested_value_props.keys.map{|k| ":#{k}"}.join(', ')} #{unrequested_value_props.length > 1 ? "props are" : "prop is"} being computed even though your partial reload did not request #{unrequested_value_props.length > 1 ? "them because they are defined as values" : "it because it is defined as a value"}. You might want to wrap these in a callable like a proc or InertiaRails::Lazy()." + end + _props = merge_props(shared_data, props).select do |key, prop| if rendering_partial_component? key.in? partial_keys diff --git a/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb b/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb new file mode 100644 index 00000000..5f3bd34b --- /dev/null +++ b/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb @@ -0,0 +1,19 @@ +class InertiaUnoptimizedPartialReloadsController < ApplicationController + inertia_share search: { query: '', results: [] } + + def index + render inertia: 'TestComponent', props: { + expensive_prop: expensive_prop, + } + end + + def with_exception + render inertia: 'TestComponent', props: { + expensive_prop: expensive_prop, + } + end + + def expensive_prop + 'Imagine this is slow to compute' + end +end diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index 42b8fb3f..0fc9ccc9 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -56,4 +56,7 @@ get 'conditional_share_show' => 'inertia_conditional_sharing#show' get 'conditional_share_edit' => 'inertia_conditional_sharing#edit' get 'conditional_share_show_with_a_problem' => 'inertia_conditional_sharing#show_with_a_problem' + + get 'unoptimized_partial_reloads' => 'inertia_unoptimized_partial_reloads#index' + get 'unoptimized_partial_reloads_with_exception' => 'inertia_unoptimized_partial_reloads#with_exception' end diff --git a/spec/inertia/partial_reload_spec.rb b/spec/inertia/partial_reload_spec.rb new file mode 100644 index 00000000..8b67bc87 --- /dev/null +++ b/spec/inertia/partial_reload_spec.rb @@ -0,0 +1,69 @@ +class FakeStdErr + attr_accessor :messages + + def initialize + @messages = [] + end + + def write(msg) + @messages << msg + end + + # Rails 5.0 + Ruby 2.6 require puts to be a public method + def puts(thing) + end +end + +RSpec.describe 'partial reloads', type: :request do + describe 'optimization guard rails' do + context 'when a non-requested prop is defined as a value' do + let (:fake_std_err) {FakeStdErr.new} + let (:warn_message) {'[InertiaRails]: WARNING! The :expensive_prop prop is being computed even though your partial reload did not request it because it is defined as a value. You might want to wrap these in a callable like a proc or InertiaRails::Lazy().'} + + around(:each) do |example| + begin + original_stderr = $stderr + $stderr = fake_std_err + + example.run + ensure + $std_err = original_stderr + end + end + + it 'only returns the requested prop' do + get unoptimized_partial_reloads_path, headers: { + 'X-Inertia' => true, + 'X-Inertia-Partial-Data' => 'search', + 'X-Inertia-Partial-Component' => 'TestComponent', + } + + expect(JSON.parse(response.body)['props'].deep_symbolize_keys).to eq({ + search: { + query: '', + results: [], + }, + }) + end + + it 'computes the non-requested prop anyway' do + expect_any_instance_of(InertiaUnoptimizedPartialReloadsController).to receive(:expensive_prop).with(any_args) + + get unoptimized_partial_reloads_path, headers: { + 'X-Inertia' => true, + 'X-Inertia-Partial-Data' => 'search', + 'X-Inertia-Partial-Component' => 'TestComponent', + } + end + + it 'emits a warning' do + get unoptimized_partial_reloads_path, headers: { + 'X-Inertia' => true, + 'X-Inertia-Partial-Data' => 'search', + 'X-Inertia-Partial-Component' => 'TestComponent', + } + expect(fake_std_err.messages[0].chomp).to(eq(warn_message)) + end + end + end +end From dbcd41ef8fec5c1a15db5b5aebfcb79c974638f1 Mon Sep 17 00:00:00 2001 From: Brian Knoles Date: Sun, 3 Nov 2024 21:36:37 -0500 Subject: [PATCH 2/7] Extract FakeStdError into its own file since it's used in multiple places now --- spec/inertia/partial_reload_spec.rb | 16 ---------------- spec/inertia/rspec_helper_spec.rb | 16 ---------------- spec/rails_helper.rb | 2 ++ spec/support/fake_std_error.rb | 15 +++++++++++++++ 4 files changed, 17 insertions(+), 32 deletions(-) create mode 100644 spec/support/fake_std_error.rb diff --git a/spec/inertia/partial_reload_spec.rb b/spec/inertia/partial_reload_spec.rb index 8b67bc87..f25643db 100644 --- a/spec/inertia/partial_reload_spec.rb +++ b/spec/inertia/partial_reload_spec.rb @@ -1,19 +1,3 @@ -class FakeStdErr - attr_accessor :messages - - def initialize - @messages = [] - end - - def write(msg) - @messages << msg - end - - # Rails 5.0 + Ruby 2.6 require puts to be a public method - def puts(thing) - end -end - RSpec.describe 'partial reloads', type: :request do describe 'optimization guard rails' do context 'when a non-requested prop is defined as a value' do diff --git a/spec/inertia/rspec_helper_spec.rb b/spec/inertia/rspec_helper_spec.rb index 12a6fb37..c122c358 100644 --- a/spec/inertia/rspec_helper_spec.rb +++ b/spec/inertia/rspec_helper_spec.rb @@ -1,21 +1,5 @@ require_relative '../../lib/inertia_rails/rspec' -class FakeStdErr - attr_accessor :messages - - def initialize - @messages = [] - end - - def write(msg) - @messages << msg - end - - # Rails 5.0 + Ruby 2.6 require puts to be a public method - def puts(thing) - end -end - RSpec.describe InertiaRails::RSpec, type: :request do describe 'correctly set up inertia tests with inertia: true', inertia: true do context 'with props' do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index f89646ee..edf7b707 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -13,6 +13,8 @@ Dir[Rails.root.join('spec', 'support', '**', '*.rb')].each { |f| require f } require_relative './support/helper_module' +require_relative './support/fake_std_error' + # Add additional requires below this line. Rails is not loaded until this point! # Requires supporting ruby files with custom matchers and macros, etc, in # spec/support/ and its subdirectories. Files matching `spec/**/*_spec.rb` are diff --git a/spec/support/fake_std_error.rb b/spec/support/fake_std_error.rb new file mode 100644 index 00000000..e9f1f7ee --- /dev/null +++ b/spec/support/fake_std_error.rb @@ -0,0 +1,15 @@ +class FakeStdErr + attr_accessor :messages + + def initialize + @messages = [] + end + + def write(msg) + @messages << msg + end + + # Rails 5.0 + Ruby 2.6 require puts to be a public method + def puts(thing) + end +end From 23f9f000ab06b9e595bc834319450172a6df689e Mon Sep 17 00:00:00 2001 From: Brian Knoles Date: Sun, 3 Nov 2024 21:37:28 -0500 Subject: [PATCH 3/7] Add spec testing warning when multiple unnecessary props are computed during a partial reload --- ...nertia_unoptimized_partial_reloads_controller.rb | 3 ++- spec/dummy/config/routes.rb | 1 + spec/inertia/partial_reload_spec.rb | 13 +++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb b/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb index 5f3bd34b..e293a8a6 100644 --- a/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb +++ b/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb @@ -7,9 +7,10 @@ def index } end - def with_exception + def with_multiple render inertia: 'TestComponent', props: { expensive_prop: expensive_prop, + another_expensive_prop: "another one", } end diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index 0fc9ccc9..41b93f31 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -59,4 +59,5 @@ get 'unoptimized_partial_reloads' => 'inertia_unoptimized_partial_reloads#index' get 'unoptimized_partial_reloads_with_exception' => 'inertia_unoptimized_partial_reloads#with_exception' + get 'unoptimized_partial_reloads_with_mutiple' => 'inertia_unoptimized_partial_reloads#with_multiple' end diff --git a/spec/inertia/partial_reload_spec.rb b/spec/inertia/partial_reload_spec.rb index f25643db..f4f5877f 100644 --- a/spec/inertia/partial_reload_spec.rb +++ b/spec/inertia/partial_reload_spec.rb @@ -3,6 +3,7 @@ context 'when a non-requested prop is defined as a value' do let (:fake_std_err) {FakeStdErr.new} let (:warn_message) {'[InertiaRails]: WARNING! The :expensive_prop prop is being computed even though your partial reload did not request it because it is defined as a value. You might want to wrap these in a callable like a proc or InertiaRails::Lazy().'} + let (:warn_message_with_multiple) {'[InertiaRails]: WARNING! The :expensive_prop, :another_expensive_prop props are being computed even though your partial reload did not request them because they are defined as values. You might want to wrap these in a callable like a proc or InertiaRails::Lazy().'} around(:each) do |example| begin @@ -48,6 +49,18 @@ } expect(fake_std_err.messages[0].chomp).to(eq(warn_message)) end + + context 'when there are multiple non-requested props defined as values' do + it 'emits a different warning' do + get unoptimized_partial_reloads_with_mutiple_path, headers: { + 'X-Inertia' => true, + 'X-Inertia-Partial-Data' => 'search', + 'X-Inertia-Partial-Component' => 'TestComponent', + } + + expect(fake_std_err.messages[0].chomp).to(eq(warn_message_with_multiple)) + end + end end end end From 62ec4b9c48e437fff0af26a29e6ad015b506f371 Mon Sep 17 00:00:00 2001 From: Brian Knoles Date: Sun, 3 Nov 2024 22:06:54 -0500 Subject: [PATCH 4/7] Refactor partial props validation --- lib/inertia_rails/renderer.rb | 36 +++++++++++++++++++++-------- spec/inertia/partial_reload_spec.rb | 4 ++-- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/lib/inertia_rails/renderer.rb b/lib/inertia_rails/renderer.rb index e95bfe34..325ec334 100644 --- a/lib/inertia_rails/renderer.rb +++ b/lib/inertia_rails/renderer.rb @@ -68,15 +68,10 @@ def merge_props(shared_data, props) end def computed_props - unrequested_value_props = props.select do |key, prop| - !prop.respond_to?(:call) && !key.in?(partial_keys) - end - - if rendering_partial_component? && unrequested_value_props.present? - InertiaRails.warn "The #{unrequested_value_props.keys.map{|k| ":#{k}"}.join(', ')} #{unrequested_value_props.length > 1 ? "props are" : "prop is"} being computed even though your partial reload did not request #{unrequested_value_props.length > 1 ? "them because they are defined as values" : "it because it is defined as a value"}. You might want to wrap these in a callable like a proc or InertiaRails::Lazy()." - end + _merged_props = merge_props(shared_data, props) + validate_partial_props(_merged_props) - _props = merge_props(shared_data, props).select do |key, prop| + _filtered_props = _merged_props.select do |key, prop| if rendering_partial_component? key.in? partial_keys else @@ -85,7 +80,7 @@ def computed_props end deep_transform_values( - _props, + _filtered_props, lambda do |prop| prop.respond_to?(:call) ? controller.instance_exec(&prop) : prop end @@ -120,5 +115,28 @@ def resolve_component(component) configuration.component_path_resolver(path: controller.controller_path, action: controller.action_name) end + + def validate_partial_props(merged_props) + return unless rendering_partial_component? + + unrequested_props = merged_props.select do |key, prop| + !key.in?(partial_keys) + end + + props_that_will_unecessarily_compute = unrequested_props.select do |key, prop| + !prop.respond_to?(:call) + end + + if props_that_will_unecessarily_compute.present? + props = props_that_will_unecessarily_compute.keys.map { |k| ":#{k}" }.join(', ') + is_plural = props_that_will_unecessarily_compute.length > 1 + verb = is_plural ? "props are" : "prop is" + pronoun = is_plural ? "them because they are defined as values" : "it because it is defined as a value" + + warning_message = "The #{props} #{verb} being computed even though your partial reload did not request #{pronoun}. You might want to wrap these in a callable like a lambda ->{} or InertiaRails::Lazy()." + + InertiaRails.warn warning_message + end + end end end diff --git a/spec/inertia/partial_reload_spec.rb b/spec/inertia/partial_reload_spec.rb index f4f5877f..8537914e 100644 --- a/spec/inertia/partial_reload_spec.rb +++ b/spec/inertia/partial_reload_spec.rb @@ -2,8 +2,8 @@ describe 'optimization guard rails' do context 'when a non-requested prop is defined as a value' do let (:fake_std_err) {FakeStdErr.new} - let (:warn_message) {'[InertiaRails]: WARNING! The :expensive_prop prop is being computed even though your partial reload did not request it because it is defined as a value. You might want to wrap these in a callable like a proc or InertiaRails::Lazy().'} - let (:warn_message_with_multiple) {'[InertiaRails]: WARNING! The :expensive_prop, :another_expensive_prop props are being computed even though your partial reload did not request them because they are defined as values. You might want to wrap these in a callable like a proc or InertiaRails::Lazy().'} + let (:warn_message) {'[InertiaRails]: WARNING! The :expensive_prop prop is being computed even though your partial reload did not request it because it is defined as a value. You might want to wrap these in a callable like a lambda ->{} or InertiaRails::Lazy().'} + let (:warn_message_with_multiple) {'[InertiaRails]: WARNING! The :expensive_prop, :another_expensive_prop props are being computed even though your partial reload did not request them because they are defined as values. You might want to wrap these in a callable like a lambda ->{} or InertiaRails::Lazy().'} around(:each) do |example| begin From 68ddff6dbc90d0a6de51b84c7812046ef4ccb007 Mon Sep 17 00:00:00 2001 From: Brian Knoles Date: Sun, 3 Nov 2024 22:38:46 -0500 Subject: [PATCH 5/7] Add configuration option to raise an exception when partial reloads are not fully optimized --- lib/inertia_rails.rb | 1 + lib/inertia_rails/configuration.rb | 2 ++ lib/inertia_rails/renderer.rb | 6 +++++- spec/dummy/app/controllers/concerns/searchable.rb | 13 +++++++++++++ .../inertia_has_searchable_controller.rb | 9 +++++++++ spec/dummy/config/routes.rb | 1 + spec/inertia/partial_reload_spec.rb | 12 ++++++++++++ 7 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 spec/dummy/app/controllers/concerns/searchable.rb create mode 100644 spec/dummy/app/controllers/inertia_has_searchable_controller.rb diff --git a/lib/inertia_rails.rb b/lib/inertia_rails.rb index 934deece..b8ea0f49 100644 --- a/lib/inertia_rails.rb +++ b/lib/inertia_rails.rb @@ -21,6 +21,7 @@ module InertiaRails class Error < StandardError; end + class UnoptimizedPartialReloadError < StandardError; end def self.deprecator # :nodoc: @deprecator ||= ActiveSupport::Deprecation.new diff --git a/lib/inertia_rails/configuration.rb b/lib/inertia_rails/configuration.rb index 02f73f05..df1a13dd 100644 --- a/lib/inertia_rails/configuration.rb +++ b/lib/inertia_rails/configuration.rb @@ -22,6 +22,8 @@ class Configuration # Used to detect version drift between server and client. version: nil, + + raise_on_unoptimized_partial_reloads: false, }.freeze OPTION_NAMES = DEFAULTS.keys.freeze diff --git a/lib/inertia_rails/renderer.rb b/lib/inertia_rails/renderer.rb index 325ec334..6fc329c8 100644 --- a/lib/inertia_rails/renderer.rb +++ b/lib/inertia_rails/renderer.rb @@ -135,7 +135,11 @@ def validate_partial_props(merged_props) warning_message = "The #{props} #{verb} being computed even though your partial reload did not request #{pronoun}. You might want to wrap these in a callable like a lambda ->{} or InertiaRails::Lazy()." - InertiaRails.warn warning_message + if configuration.raise_on_unoptimized_partial_reloads + raise InertiaRails::UnoptimizedPartialReloadError, warning_message + else + InertiaRails.warn warning_message + end end end end diff --git a/spec/dummy/app/controllers/concerns/searchable.rb b/spec/dummy/app/controllers/concerns/searchable.rb new file mode 100644 index 00000000..61f5b0e4 --- /dev/null +++ b/spec/dummy/app/controllers/concerns/searchable.rb @@ -0,0 +1,13 @@ +module Searchable + extend ActiveSupport::Concern + + included do + inertia_config(raise_on_unoptimized_partial_reloads: true) + + inertia_share do + { + search: { query: '', results: [] } + } + end + end +end diff --git a/spec/dummy/app/controllers/inertia_has_searchable_controller.rb b/spec/dummy/app/controllers/inertia_has_searchable_controller.rb new file mode 100644 index 00000000..793ba678 --- /dev/null +++ b/spec/dummy/app/controllers/inertia_has_searchable_controller.rb @@ -0,0 +1,9 @@ +class InertiaHasSearchableController < ApplicationController + include Searchable + + def index + render inertia: 'TestComponent', props: { + unrequested_prop: 'This will always compute even when not requested in a partial reload', + } + end +end diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index 41b93f31..fcb87dcb 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -60,4 +60,5 @@ get 'unoptimized_partial_reloads' => 'inertia_unoptimized_partial_reloads#index' get 'unoptimized_partial_reloads_with_exception' => 'inertia_unoptimized_partial_reloads#with_exception' get 'unoptimized_partial_reloads_with_mutiple' => 'inertia_unoptimized_partial_reloads#with_multiple' + get 'has_searchable' => 'inertia_has_searchable#index' end diff --git a/spec/inertia/partial_reload_spec.rb b/spec/inertia/partial_reload_spec.rb index 8537914e..ba7079ad 100644 --- a/spec/inertia/partial_reload_spec.rb +++ b/spec/inertia/partial_reload_spec.rb @@ -61,6 +61,18 @@ expect(fake_std_err.messages[0].chomp).to(eq(warn_message_with_multiple)) end end + + context 'when the controller is configured to raise_on_unoptimized_partial_reloads' do + it 'emits a warning' do + expect { + get has_searchable_path, headers: { + 'X-Inertia' => true, + 'X-Inertia-Partial-Data' => 'search', + 'X-Inertia-Partial-Component' => 'TestComponent', + } + }.to raise_error(InertiaRails::UnoptimizedPartialReloadError, /unrequested_prop/) + end + end end end end From 7555b1a0ae89dd85aebd2abe6f149372b80155d6 Mon Sep 17 00:00:00 2001 From: Brian Knoles Date: Sun, 3 Nov 2024 22:48:30 -0500 Subject: [PATCH 6/7] Add spec to ensure we don't warn about properly optimized partial reload props --- .../inertia_unoptimized_partial_reloads_controller.rb | 1 + spec/inertia/partial_reload_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb b/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb index e293a8a6..32eaaf64 100644 --- a/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb +++ b/spec/dummy/app/controllers/inertia_unoptimized_partial_reloads_controller.rb @@ -11,6 +11,7 @@ def with_multiple render inertia: 'TestComponent', props: { expensive_prop: expensive_prop, another_expensive_prop: "another one", + callable_prop: -> { 'This is a callable prop' }, } end diff --git a/spec/inertia/partial_reload_spec.rb b/spec/inertia/partial_reload_spec.rb index ba7079ad..6ac81ed3 100644 --- a/spec/inertia/partial_reload_spec.rb +++ b/spec/inertia/partial_reload_spec.rb @@ -50,6 +50,16 @@ expect(fake_std_err.messages[0].chomp).to(eq(warn_message)) end + it 'does not warn about callable props' do + get unoptimized_partial_reloads_path, headers: { + 'X-Inertia' => true, + 'X-Inertia-Partial-Data' => 'search', + 'X-Inertia-Partial-Component' => 'TestComponent', + } + + expect(fake_std_err.messages[0].chomp).not_to include('callable_prop') + end + context 'when there are multiple non-requested props defined as values' do it 'emits a different warning' do get unoptimized_partial_reloads_with_mutiple_path, headers: { From 9931f885f914e54dfd8856d6bb379d96bbbb8aad Mon Sep 17 00:00:00 2001 From: Brian Knoles Date: Sun, 3 Nov 2024 22:50:48 -0500 Subject: [PATCH 7/7] Fix spec description --- spec/inertia/partial_reload_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/inertia/partial_reload_spec.rb b/spec/inertia/partial_reload_spec.rb index 6ac81ed3..52b659e4 100644 --- a/spec/inertia/partial_reload_spec.rb +++ b/spec/inertia/partial_reload_spec.rb @@ -73,7 +73,7 @@ end context 'when the controller is configured to raise_on_unoptimized_partial_reloads' do - it 'emits a warning' do + it 'raises an error' do expect { get has_searchable_path, headers: { 'X-Inertia' => true,