Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize ViewComponent::Config with defaults before framework load #1774

Merged
merged 5 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ nav_order: 5

## main

* Initialize ViewComponent::Config with defaults before framework load.

*Simon Fish*

* Add 3.2 to the list of Ruby CI versions

*Igor Drozdov*
Expand Down
6 changes: 1 addition & 5 deletions lib/view_component/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,8 @@ class << self
#
# @return [ActiveSupport::OrderedOptions]
def config
@config ||= ActiveSupport::OrderedOptions.new
ViewComponent::Config.current
end

# Replaces the entire config. You shouldn't need to use this directly
# unless you're building a `ViewComponent::Config` elsewhere.
attr_writer :config
Comment on lines -28 to -30
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to regenerate the YARD docs after removing this.

end

include ViewComponent::InlineTemplate
Expand Down
8 changes: 8 additions & 0 deletions lib/view_component/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,14 @@ def default_generate_options
end
end

# @!attribute current
# @return [ViewComponent::Config]
# Returns the current ViewComponent::Config. This is persisted against this
# class so that config options remain accessible before the rest of
# ViewComponent has loaded. Defaults to an instance of ViewComponent::Config
# with all other documented defaults set.
class_attribute :current, default: defaults, instance_predicate: false
Comment on lines +170 to +176
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, we need to regenerate the YARD docs to include this.


def initialize
@config = self.class.defaults
end
Expand Down
7 changes: 4 additions & 3 deletions lib/view_component/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

module ViewComponent
class Engine < Rails::Engine # :nodoc:
config.view_component = ViewComponent::Config.defaults
config.view_component = ViewComponent::Config.current

rake_tasks do
load "view_component/rails/tasks/view_component.rake"
Expand All @@ -15,6 +15,9 @@ class Engine < Rails::Engine # :nodoc:
initializer "view_component.set_configs" do |app|
options = app.config.view_component

%i[generate preview_controller preview_route show_previews_source].each do |config_option|
options[config_option] ||= ViewComponent::Base.public_send(config_option)
end
options.instrumentation_enabled = false if options.instrumentation_enabled.nil?
options.render_monkey_patch_enabled = true if options.render_monkey_patch_enabled.nil?
options.show_previews = (Rails.env.development? || Rails.env.test?) if options.show_previews.nil?
Expand All @@ -37,8 +40,6 @@ class Engine < Rails::Engine # :nodoc:

initializer "view_component.enable_instrumentation" do |app|
ActiveSupport.on_load(:view_component) do
Base.config = app.config.view_component

if app.config.view_component.instrumentation_enabled.present?
# :nocov: Re-executing the below in tests duplicates initializers and causes order-dependent failures.
ViewComponent::Base.prepend(ViewComponent::Instrumentation)
Expand Down
2 changes: 1 addition & 1 deletion lib/view_component/preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

module ViewComponent # :nodoc:
class Preview
include Rails.application.routes.url_helpers if defined?(Rails.application.routes.url_helpers)
include Rails.application.routes.url_helpers if defined?(Rails) && Rails.application.present?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think #1774 (review) was resolved correctly - I think ultimately this change can be reverted.

Originally @boardfish had committed

diff --git a/lib/view_component/preview.rb b/lib/view_component/preview.rb
index e80087404..3176e4ecd 100644
--- a/lib/view_component/preview.rb
+++ b/lib/view_component/preview.rb
@@ -4,7 +4,7 @@
 
 module ViewComponent # :nodoc:
   class Preview
-    include Rails.application.routes.url_helpers
+    include Rails.application.routes.url_helpers if Rails && Rails.application.present?
     include ActionView::Helpers::TagHelper
     include ActionView::Helpers::AssetTagHelper
     extend ActiveSupport::DescendantsTracker

in e4268dc before 0d6564a had landed in master. When the PR was later rebased, this change should've been dropped, as @boardfish realized in #1774 (comment).

Technically, it's possible (though admittedly weird) to have an environment where Rails.application is present even if Rails.application.routes.url_helpers is not defined - and the change introduced here would no longer handle that correctly.

include ActionView::Helpers::TagHelper
include ActionView::Helpers::AssetTagHelper
extend ActiveSupport::DescendantsTracker
Expand Down
2 changes: 1 addition & 1 deletion test/sandbox/test/config_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def test_all_methods_are_documented
end
options_defined_on_instance = Set[*default_options, *accessors]
assert options_defined_on_instance.subset?(Set[*configuration_methods_to_document.map(&:name)]),
"Not all configuration options are documented: #{configuration_methods_to_document.map(&:name) - options_defined_on_instance.to_a}"
"Not all configuration options are documented: #{options_defined_on_instance.to_a - configuration_methods_to_document.map(&:name)}"
assert configuration_methods_to_document.map(&:docstring).all?(&:present?),
"Configuration options are missing docstrings."
end
Expand Down
Loading