From 0729cff965f2b6ed7f14f94743a86fc3f2e35d67 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 22 Nov 2022 14:28:01 -0800 Subject: [PATCH 1/7] Improve performance of deny_* methods by not raising in prod environments --- app/components/primer/component.rb | 4 ++-- demo/Gemfile | 4 ++-- demo/Gemfile.lock | 11 ++++++----- test/components/component_test.rb | 31 ++++++++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/app/components/primer/component.rb b/app/components/primer/component.rb index 066976ffd9..abc539c57e 100644 --- a/app/components/primer/component.rb +++ b/app/components/primer/component.rb @@ -121,11 +121,11 @@ def deny_tag_argument(**arguments) end def should_raise_error? - raise_on_invalid_options? && !ENV["PRIMER_WARNINGS_DISABLED"] + !Rails.env.production? && raise_on_invalid_options? && !ENV["PRIMER_WARNINGS_DISABLED"] end def should_raise_aria_error? - raise_on_invalid_aria? && !ENV["PRIMER_WARNINGS_DISABLED"] + !Rails.env.production? && raise_on_invalid_aria? && !ENV["PRIMER_WARNINGS_DISABLED"] end end end diff --git a/demo/Gemfile b/demo/Gemfile index 333cedb61c..6199fa5184 100644 --- a/demo/Gemfile +++ b/demo/Gemfile @@ -40,8 +40,8 @@ gem "lookbook", "~> 1.3.4" unless rails_version.to_f < 7 group :development do # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring - gem "spring" - gem "spring-watcher-listen", "~> 2.0.0" + gem "spring", "~> 4.0" + gem "spring-watcher-listen", "~> 2.1" gem "hotwire-livereload", "~> 1.1" # Use JavaScript with ESM import maps [https://github.com/rails/importmap-rails] gem "importmap-rails" diff --git a/demo/Gemfile.lock b/demo/Gemfile.lock index 9cf21a5299..2267b240e4 100644 --- a/demo/Gemfile.lock +++ b/demo/Gemfile.lock @@ -337,10 +337,10 @@ GEM jwt (>= 1.5, < 3.0) multi_json (~> 1.10) sorbet-runtime-stub (0.2.0) - spring (2.1.1) - spring-watcher-listen (2.0.1) + spring (4.1.0) + spring-watcher-listen (2.1.0) listen (>= 2.7, < 4.0) - spring (>= 1.2, < 3.0) + spring (>= 4) sprockets (4.1.1) concurrent-ruby (~> 1.0) rack (> 1, < 3) @@ -378,6 +378,7 @@ GEM PLATFORMS x86_64-darwin-19 + x86_64-darwin-20 x86_64-linux DEPENDENCIES @@ -399,8 +400,8 @@ DEPENDENCIES rack-cors railties (= 7.0.3) rake (~> 13.0) - spring - spring-watcher-listen (~> 2.0.0) + spring (~> 4.0) + spring-watcher-listen (~> 2.1) sprockets sprockets-rails stimulus-rails diff --git a/test/components/component_test.rb b/test/components/component_test.rb index e28a172a27..f8cc5c3f0b 100644 --- a/test/components/component_test.rb +++ b/test/components/component_test.rb @@ -184,4 +184,35 @@ def test_deprecated_components_by_status_match_list assert_empty(deprecated_by_status - deprecated_by_list, "Please make sure that components are officially deprecated by setting the `status :deprecated` within the component file.\nMake sure to provide an alternative component for each deprecated component in Primer::Deprecations::DEPRECATED_COMPONENTS (lib/primer/deprecations.rb). If there is no alternative to suggest, set the value to nil.") end + + class DenyComponent < Primer::Component + def initialize(**system_arguments) + deny_single_argument(:class, "Use `classes` instead.", **system_arguments) + deny_aria_key(:label, "Don't use labels?", **system_arguments) + end + + def call + "

foo

".html_safe + end + end + + def test_deny_single_argument_does_not_raise_in_production + with_raise_on_invalid_options(true) do + assert_raises(ArgumentError) { DenyComponent.new(class: "foo") } + + Rails.stub(:env, "production".inquiry) do + DenyComponent.new(class: "foo") + end + end + end + + def test_deny_aria_key_does_not_raise_in_production + with_raise_on_invalid_aria(true) do + assert_raises(ArgumentError) { DenyComponent.new(aria: { label: "foo" }) } + + Rails.stub(:env, "production".inquiry) do + DenyComponent.new(aria: { label: "foo" }) + end + end + end end From 25c495b7b029c49ec46605af8a08b0348cb28dfa Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 22 Nov 2022 14:32:55 -0800 Subject: [PATCH 2/7] Add changeset --- .changeset/gorgeous-spies-hang.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/gorgeous-spies-hang.md diff --git a/.changeset/gorgeous-spies-hang.md b/.changeset/gorgeous-spies-hang.md new file mode 100644 index 0000000000..aed695c427 --- /dev/null +++ b/.changeset/gorgeous-spies-hang.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Improve performance of the deny\_\* methods From d765838308d32962ad1aea8d6f81a3020052b8ef Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 22 Nov 2022 14:35:17 -0800 Subject: [PATCH 3/7] Fix linting issues --- test/components/component_test.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/components/component_test.rb b/test/components/component_test.rb index f8cc5c3f0b..06cd660c81 100644 --- a/test/components/component_test.rb +++ b/test/components/component_test.rb @@ -200,9 +200,11 @@ def test_deny_single_argument_does_not_raise_in_production with_raise_on_invalid_options(true) do assert_raises(ArgumentError) { DenyComponent.new(class: "foo") } + # rubocop:disable Rails/Inquiry Rails.stub(:env, "production".inquiry) do DenyComponent.new(class: "foo") end + # rubocop:enable Rails/Inquiry end end @@ -210,9 +212,11 @@ def test_deny_aria_key_does_not_raise_in_production with_raise_on_invalid_aria(true) do assert_raises(ArgumentError) { DenyComponent.new(aria: { label: "foo" }) } + # rubocop:disable Rails/Inquiry Rails.stub(:env, "production".inquiry) do DenyComponent.new(aria: { label: "foo" }) end + # rubocop:enable Rails/Inquiry end end end From f5467e016418ee3fed7df3d8aa2f5d5ac9715ad5 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 22 Nov 2022 15:56:51 -0800 Subject: [PATCH 4/7] Add test codifying speed improvement --- demo/config/application.rb | 1 + test/components/component_test.rb | 19 +++----------- test/components/primer/deny_component.rb | 14 ++++++++++ test/performance/bench_deny.rb | 26 +++++++++++++++++++ test/test_helpers/ips_test_helper.rb | 33 ++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 15 deletions(-) create mode 100644 test/components/primer/deny_component.rb create mode 100644 test/performance/bench_deny.rb create mode 100644 test/test_helpers/ips_test_helper.rb diff --git a/demo/config/application.rb b/demo/config/application.rb index 854c21a0c6..e1f851ede3 100644 --- a/demo/config/application.rb +++ b/demo/config/application.rb @@ -29,6 +29,7 @@ class Application < Rails::Application config.view_component.preview_paths << Rails.root.join("../previews") config.autoload_paths << Rails.root.join("../test/forms") + config.autoload_paths << Rails.root.join("../test/components") config.action_dispatch.default_headers.clear diff --git a/test/components/component_test.rb b/test/components/component_test.rb index 06cd660c81..940f700dc0 100644 --- a/test/components/component_test.rb +++ b/test/components/component_test.rb @@ -185,24 +185,13 @@ def test_deprecated_components_by_status_match_list assert_empty(deprecated_by_status - deprecated_by_list, "Please make sure that components are officially deprecated by setting the `status :deprecated` within the component file.\nMake sure to provide an alternative component for each deprecated component in Primer::Deprecations::DEPRECATED_COMPONENTS (lib/primer/deprecations.rb). If there is no alternative to suggest, set the value to nil.") end - class DenyComponent < Primer::Component - def initialize(**system_arguments) - deny_single_argument(:class, "Use `classes` instead.", **system_arguments) - deny_aria_key(:label, "Don't use labels?", **system_arguments) - end - - def call - "

foo

".html_safe - end - end - def test_deny_single_argument_does_not_raise_in_production with_raise_on_invalid_options(true) do - assert_raises(ArgumentError) { DenyComponent.new(class: "foo") } + assert_raises(ArgumentError) { Primer::DenyComponent.new(class: "foo") } # rubocop:disable Rails/Inquiry Rails.stub(:env, "production".inquiry) do - DenyComponent.new(class: "foo") + Primer::DenyComponent.new(class: "foo") end # rubocop:enable Rails/Inquiry end @@ -210,11 +199,11 @@ def test_deny_single_argument_does_not_raise_in_production def test_deny_aria_key_does_not_raise_in_production with_raise_on_invalid_aria(true) do - assert_raises(ArgumentError) { DenyComponent.new(aria: { label: "foo" }) } + assert_raises(ArgumentError) { Primer::DenyComponent.new(aria: { label: "foo" }) } # rubocop:disable Rails/Inquiry Rails.stub(:env, "production".inquiry) do - DenyComponent.new(aria: { label: "foo" }) + Primer::DenyComponent.new(aria: { label: "foo" }) end # rubocop:enable Rails/Inquiry end diff --git a/test/components/primer/deny_component.rb b/test/components/primer/deny_component.rb new file mode 100644 index 0000000000..e334edc916 --- /dev/null +++ b/test/components/primer/deny_component.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Primer + class DenyComponent < Primer::Component + def initialize(**system_arguments) + deny_single_argument(:class, "Use `classes` instead.", **system_arguments) + deny_aria_key(:label, "Don't use labels?", **system_arguments) + end + + def call + "

foo

".html_safe + end + end +end diff --git a/test/performance/bench_deny.rb b/test/performance/bench_deny.rb new file mode 100644 index 0000000000..1c9ed86411 --- /dev/null +++ b/test/performance/bench_deny.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require "benchmark/ips" +require "minitest" +require "minitest/benchmark" +require "test_helper" +require "test_helpers/ips_test_helper" + +class BenchDeny < Minitest::Benchmark + include Primer::IPSTestHelper + + def bench_deny_single_argument + non_prod_results = measure_ips { Primer::DenyComponent.new } + + # rubocop:disable Rails/Inquiry + prod_results = Rails.stub(:env, "production".inquiry) do + measure_ips { Primer::DenyComponent.new } + end + # rubocop:enable Rails/Inquiry + + assert_more_ips( + prod_results, non_prod_results, + "Primer::Component#deny_single_argument is supposed to be more performant in production" + ) + end +end diff --git a/test/test_helpers/ips_test_helper.rb b/test/test_helpers/ips_test_helper.rb new file mode 100644 index 0000000000..dfd5a8c1b4 --- /dev/null +++ b/test/test_helpers/ips_test_helper.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# This code is exercised by rake bench +# :nocov: + +require "benchmark/ips" + +module Primer + module IPSTestHelper + def measure_ips + suppress_output do + Benchmark.ips { |x| x.report { yield } } + end + end + + def assert_more_ips(faster, slower, msg = nil) + assert faster.data.first[:ips] > slower.data.first[:ips], msg + end + + private + + def suppress_output + original_stderr = $stderr.clone + original_stdout = $stdout.clone + $stderr.reopen(File.new("/dev/null", "w")) + $stdout.reopen(File.new("/dev/null", "w")) + yield + ensure + $stdout.reopen(original_stdout) + $stderr.reopen(original_stderr) + end + end +end From e1a533872c63c0dcbf8641c014553e12e3bf54e2 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 22 Nov 2022 15:58:13 -0800 Subject: [PATCH 5/7] Remove unnecessary require --- test/performance/bench_deny.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/test/performance/bench_deny.rb b/test/performance/bench_deny.rb index 1c9ed86411..1d3128cc38 100644 --- a/test/performance/bench_deny.rb +++ b/test/performance/bench_deny.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require "benchmark/ips" require "minitest" require "minitest/benchmark" require "test_helper" From 482ae3bd6126fc8b57e4a009ca137af80b9448c9 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 22 Nov 2022 16:01:02 -0800 Subject: [PATCH 6/7] Fix lint issues --- test/test_helpers/ips_test_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_helpers/ips_test_helper.rb b/test/test_helpers/ips_test_helper.rb index dfd5a8c1b4..98a03f288f 100644 --- a/test/test_helpers/ips_test_helper.rb +++ b/test/test_helpers/ips_test_helper.rb @@ -7,9 +7,9 @@ module Primer module IPSTestHelper - def measure_ips + def measure_ips(&block) suppress_output do - Benchmark.ips { |x| x.report { yield } } + Benchmark.ips { |x| x.report(&block) } end end From b70528cdaa8681024ca911ce798dfd47144fb1a6 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 23 Nov 2022 15:28:19 -0800 Subject: [PATCH 7/7] Move test components into their own dir --- demo/config/application.rb | 2 +- test/{ => test_helpers}/components/primer/deny_component.rb | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename test/{ => test_helpers}/components/primer/deny_component.rb (100%) diff --git a/demo/config/application.rb b/demo/config/application.rb index e1f851ede3..5910c987d1 100644 --- a/demo/config/application.rb +++ b/demo/config/application.rb @@ -29,7 +29,7 @@ class Application < Rails::Application config.view_component.preview_paths << Rails.root.join("../previews") config.autoload_paths << Rails.root.join("../test/forms") - config.autoload_paths << Rails.root.join("../test/components") + config.autoload_paths << Rails.root.join("../test/test_helpers/components") config.action_dispatch.default_headers.clear diff --git a/test/components/primer/deny_component.rb b/test/test_helpers/components/primer/deny_component.rb similarity index 100% rename from test/components/primer/deny_component.rb rename to test/test_helpers/components/primer/deny_component.rb