ViewComponent / view_component

A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
https://viewcomponent.org
MIT License
3.31k stars 434 forks source link

TypeError: no implicit conversion of nil into String #1565

Open krsyoung opened 2 years ago

krsyoung commented 2 years ago

Thanks for all of your work on 2.75.0! Our gem seems to be less appreciative though...

We are hitting an issue with 2.75.0 causing TypeError: no implicit conversion of nil into String in our matey gem. The problem does not occur in 2.74.1.

For some reason in base.rb ViewComponent::Base.config.view_component_path is nil which causes Regex.quote to throw an exception. Upon a little further inspection, the whole config object is empty:

(ruby) ViewComponent::Base.config
{}

If I add something like the following above base.rb#509 it seems to work :-)

ViewComponent::Base.config = ViewComponent::Config.new

I'm still digging around but wanted to post in case others are experiencing the same issue.

Steps to reproduce

  1. clone https://github.com/harled/matey
  2. Switch to 2.75.0 in matey.gemspec
  3. bin/setup
  4. rake spec

Expected behavior

Expect the test cases to pass as with 2.74.1

Actual behavior

Test cases are not happy!

TypeError: no implicit conversion of nil into String
/usr/local/rvm/gems/default/gems/view_component-2.75.0/lib/view_component/base.rb:509:in `quote'
/usr/local/rvm/gems/default/gems/view_component-2.75.0/lib/view_component/base.rb:509:in `inherited'
/workspaces/matey/app/components/application_component.rb:4:in `<top (required)>'
/workspaces/matey/lib/matey.rb:5:in `require_relative'
/workspaces/matey/lib/matey.rb:5:in `<top (required)>'
/workspaces/matey/spec/sample/config/application.rb:15:in `<top (required)>'

System configuration

Rails version: 7.0.4

Ruby version: ruby 3.1.2p20

Gem version: 2.75.0

boardfish commented 2 years ago

I think the change to lib/view_component/engine.rb and base.rb between these versions might be responsible. This was introduced in #1528.

I'm surprised that the tests that maintain compatibility between ViewComponent::Base.config and Rails.application.config.view_component allowed this - it seems to suggest those will need to be updated so that they fail for this instance.

There's an interesting problem here. Rails' own config is the user's endpoint for making config changes, but ViewComponent::Base is what's used by gems that integrate with it, such as Matey, since they can't rely on Rails having loaded. That's why we need to mirror the two, but as we can see in #1528, it would appear that that's in conflict with us using a shared config object. @jdelStrother, any chance you might be able to address this? I wonder if there's a better place in the initialization flow for us to mirror the two configs such that we're not loading ActionView too early, or whether there's a different way that we can make Base.config and Rails.application.config.view_component point to the same thing.

jcoyne commented 2 years ago

We're also seeing this problem with the Blacklight Rails engine.

krsyoung commented 2 years ago

Thanks for the digging @boardfish! We (from a gem perspective) are also happy to try to build on top of ViewComponent a different way if that might lead to a better future / less complexity from your side. I'm just not sure how that might look at the moment. 🤔

jdelStrother commented 2 years ago

Hm, sorry for the issues - I don't think I considered loading this outside of Rails. I'll try and take a look at this sometime next week, if noone else beats me to it.

jcoyne commented 2 years ago

I found that I can resolve this problem by adding this to my engine's base component class:

     def self.inherited(subclass)
       subclass.config = ViewComponent::Base.config
       super
    end
krsyoung commented 2 years ago

Thanks for sharing @jcoyne .. I'm not having as much luck (looking into what I'm doing differently).

require "view_component"
require "ahoy_matey"

class ApplicationComponent < ViewComponent::Base
  include ActiveModel::Validations

  # workaround for https://github.com/ViewComponent/view_component/issues/1565#issuecomment-1302397965
  class << self
    def inherited(subclass)
      subclass.config = ViewComponent::Config.new
      super
    end
  end
end

For me it still blows up on class ApplicationComponent < ViewComponent::Base.

jdelStrother commented 2 years ago

I think one possible easy fix is like this -

diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb
index f1bc391..11f712a 100644
--- a/lib/view_component/base.rb
+++ b/lib/view_component/base.rb
@@ -20,7 +20,7 @@ module ViewComponent
       delegate(*ViewComponent::Config.defaults.keys, to: :config)

       def config
-        @config ||= ActiveSupport::OrderedOptions.new
+        @config ||= ViewComponent::Config.defaults
       end
       attr_writer :config
     end

Then both Rails.application.config.view_component and VC::Base.config will default to the values in VC::Config.defaults.

Will try and add some tests demoing the empty config outside of rails.

jdelStrother commented 2 years ago

Hm, I think the above fix is fine for the simple case as long as matey/blacklight/etc don't need to mess with ViewComponent config. If they do try and set, eg, ViewComponent::Base.config.view_component_path, that'll get lost after Rails initialization completes here - https://github.com/github/view_component/blob/fa83442646eff70f1801d6885a3fc8c430a1ce68/lib/view_component/engine.rb#L39

krsyoung commented 2 years ago

@jdelStrother thanks! Just confirming that this change does indeed make matey happy. Thank you for digging into this!

I don't foresee (instantly jinxed) us modifying any of the base configurations as they exist today. In the event we did, might a Rails initializer work (I can try this later today)?

donapieppo commented 1 year ago

I have the same error on version 3.0.0 when a gem uses view_component. It raises

view_component-3.0.0/lib/view_component/base.rb:455:in 'quote': no implicit conversion of nil into String (TypeError)

To fix it I surrounded that code with if defined?(Rails) && Rails.application because ViewComponent::Base.config.view_component_path is nil when view_component in required in a gem.

So it becames

if defined?(Rails) && Rails.application
  child.virtual_path = child.source_location.gsub(
    /(.*#{Regexp.quote(ViewComponent::Base.config.view_component_path)})|(\.rb)/, ""
  )
end