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

boardfish · 2023-06-18T10:25:14Z

This PR introduces ViewComponent::Config.current, which should not be used directly by consumers of ViewComponent, but is used as the common object from which ViewComponent::Base and Rails.application.config.view_component will load their config once each is ready. It'll make config available from the very start with the intent of not relying upon the framework to find these options - we can manually set anything reliant on Action View, for example, within the app or engine config.

#1659 made the config object empty before ViewComponent and Rails were initialised. This would mean that code within ViewComponent that relied on configured defaults would break, resulting in issues like #1741. Since ViewComponent::Config can be loaded independently of Action View, this should hopefully no longer be the case.

If you were affected by #1741, please test against this branch to see if it fixes your issue. Thanks for your patience!

camertron

Hey @boardfish thanks for working on this. I don't understand how current get set - there doesn't appear to be any call to it?

Spone · 2023-06-26T19:32:14Z

Hey @boardfish thanks for working on this. I don't understand how current get set - there doesn't appear to be any call to it?

I think the defaults class method is called here:

class_attribute :current, default: defaults, instance_predicate: false

This reverts commit 01510f3.

This commit introduces `ViewComponent::Config.current`, which should not be used directly by consumers of ViewComponent, but is used as the common thread from which `ViewComponent::Base` and `Rails.application.config.view_component` will load their config once each is ready. It'll make config available from the very start with the intent of not relying upon the framework to find these options - we can manually set anything reliant on Action View, for example, within app/engine config.

boardfish · 2023-07-19T16:01:02Z

Hey folks, is there anything else you'd like me to do with this so we can get it shipped? I'm aware there are now multiple issues that this will fix.

Spone

Looks good to me!

BlakeWilliams · 2023-07-22T15:32:17Z

lib/view_component/preview.rb

@@ -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 Rails && Rails.application.present?


Do we still need the defined? check to support non-Rails environments?

Yeah, I think this must've fallen through in a rebate 🧐 I'll make sure to fix it before it gets merged.

camertron · 2023-07-26T20:05:17Z

@Spone looks like there's a coverage issue as well 😦

Spone · 2023-08-09T20:50:41Z

Do you know what's happening with the coverage @boardfish?

boardfish · 2023-08-10T08:24:18Z

I think it wants ViewComponent::Base#config= to be tested, but I think that method might even be able to be removed.

ryansdwilson · 2023-08-21T21:23:02Z

Thank you all for your work on this! I ran into this issue today and went down a rabbit hole trying to figure it out. I really appreciate all the work you're doing to maintain VC.

boardfish · 2023-08-24T10:09:00Z

Thanks for wrapping this up @camertron, and sorry I haven't been able to dedicate the time to it! 😅 🙏

rmacklin

I'll open a new PR to address this feedback since this one is already merged

rmacklin · 2023-08-24T18:20:54Z

lib/view_component/preview.rb

@@ -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?


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.

rmacklin · 2023-08-24T18:23:36Z

lib/view_component/base.rb

-      # Replaces the entire config. You shouldn't need to use this directly
-      # unless you're building a `ViewComponent::Config` elsewhere.
-      attr_writer :config


We need to regenerate the YARD docs after removing this.

rmacklin · 2023-08-24T18:28:08Z

lib/view_component/config.rb

+    # @!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


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

…iewComponent#1774) * Revert "Avoid loading ActionView::Base during initialization (ViewComponent#1659)" This reverts commit 01510f3. * Initialize ViewComponent::Config with defaults before framework load This commit introduces `ViewComponent::Config.current`, which should not be used directly by consumers of ViewComponent, but is used as the common thread from which `ViewComponent::Base` and `Rails.application.config.view_component` will load their config once each is ready. It'll make config available from the very start with the intent of not relying upon the framework to find these options - we can manually set anything reliant on Action View, for example, within app/engine config. * Only include URL helpers if app is loaded * Get this across the finish line --------- Co-authored-by: Cameron Dutro <camertron@gmail.com>

boardfish force-pushed the fix-virtual-path-nil-viewcomponent-path branch from cb03c95 to a9fb769 Compare June 18, 2023 10:30

boardfish marked this pull request as ready for review June 18, 2023 10:31

boardfish requested review from Spone and camertron June 18, 2023 10:31

boardfish linked an issue Jun 18, 2023 that may be closed by this pull request

ViewComponent::Base.config.view_component_path nil error #1741

Closed

boardfish mentioned this pull request Jun 20, 2023

ViewComponent::Base.config.view_component_path nil error #1741

Closed

boardfish requested a review from juanmanuelramallo as a code owner June 21, 2023 08:05

boardfish force-pushed the fix-virtual-path-nil-viewcomponent-path branch from 7a89729 to e4268dc Compare June 21, 2023 08:18

camertron reviewed Jun 26, 2023

View reviewed changes

boardfish mentioned this pull request Jul 19, 2023

ViewComponent::Base.view_component_path defaulting to nil breaks rails stats #1804

Closed

boardfish added 3 commits July 19, 2023 16:51

Revert "Avoid loading ActionView::Base during initialization (#1659)"

0dd2cf9

This reverts commit 01510f3.

Only include URL helpers if app is loaded

a3ed95f

boardfish force-pushed the fix-virtual-path-nil-viewcomponent-path branch from e4268dc to a3ed95f Compare July 19, 2023 15:53

boardfish requested a review from camertron July 19, 2023 15:59

boardfish linked an issue Jul 19, 2023 that may be closed by this pull request

ViewComponent::Base.view_component_path defaulting to nil breaks rails stats #1804

Closed

Spone approved these changes Jul 19, 2023

View reviewed changes

BlakeWilliams approved these changes Jul 22, 2023

View reviewed changes

camertron self-assigned this Aug 23, 2023

camertron added 2 commits August 23, 2023 14:28

Get this across the finish line

fdceb20

Merge upstream

0cd09a7

camertron approved these changes Aug 23, 2023

View reviewed changes

camertron merged commit 6979de9 into main Aug 23, 2023
37 checks passed

camertron deleted the fix-virtual-path-nil-viewcomponent-path branch August 23, 2023 22:06

rmacklin reviewed Aug 24, 2023

View reviewed changes

rmacklin mentioned this pull request Aug 24, 2023

Fix API documentation and revert unnecessary change in preview.rb #1827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

boardfish commented Jun 18, 2023 •

edited

Loading

camertron left a comment

Spone commented Jun 26, 2023 •

edited

Loading

boardfish commented Jul 19, 2023

Spone left a comment

BlakeWilliams Jul 22, 2023

boardfish Jul 23, 2023

camertron commented Jul 26, 2023

Spone commented Aug 9, 2023

boardfish commented Aug 10, 2023

ryansdwilson commented Aug 21, 2023

boardfish commented Aug 24, 2023

rmacklin left a comment

rmacklin Aug 24, 2023

rmacklin Aug 24, 2023

rmacklin Aug 24, 2023

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

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

Conversation

boardfish commented Jun 18, 2023 • edited Loading

camertron left a comment

Choose a reason for hiding this comment

Spone commented Jun 26, 2023 • edited Loading

boardfish commented Jul 19, 2023

Spone left a comment

Choose a reason for hiding this comment

BlakeWilliams Jul 22, 2023

Choose a reason for hiding this comment

boardfish Jul 23, 2023

Choose a reason for hiding this comment

camertron commented Jul 26, 2023

Spone commented Aug 9, 2023

boardfish commented Aug 10, 2023

ryansdwilson commented Aug 21, 2023

boardfish commented Aug 24, 2023

rmacklin left a comment

Choose a reason for hiding this comment

rmacklin Aug 24, 2023

Choose a reason for hiding this comment

rmacklin Aug 24, 2023

Choose a reason for hiding this comment

rmacklin Aug 24, 2023

Choose a reason for hiding this comment

boardfish commented Jun 18, 2023 •

edited

Loading

Spone commented Jun 26, 2023 •

edited

Loading