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

TypeError: no implicit conversion of nil into String #1565

Open
krsyoung opened this issue Nov 3, 2022 · 10 comments
Open

TypeError: no implicit conversion of nil into String #1565

krsyoung opened this issue Nov 3, 2022 · 10 comments

Comments

@krsyoung
Copy link

krsyoung commented Nov 3, 2022

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
Copy link
Collaborator

boardfish commented Nov 3, 2022

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
Copy link
Collaborator

jcoyne commented Nov 3, 2022

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

@krsyoung
Copy link
Author

krsyoung commented Nov 3, 2022

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
Copy link
Contributor

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
Copy link
Collaborator

jcoyne commented Nov 3, 2022

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
Copy link
Author

krsyoung commented Nov 3, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

krsyoung commented Nov 6, 2022

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

jdelStrother added a commit to jdelStrother/view_component that referenced this issue Nov 6, 2022
If the Railtie initialization hasn't yet run (or if it won't run at all
because it's being used outside of a Rails app), and a gem tries to
subclass ViewComponent::Base, it would fail in Base.inherited due to
config.view_component_path being nil.

Fixes ViewComponent#1565
jdelStrother added a commit to jdelStrother/view_component that referenced this issue Nov 7, 2022
If the Railtie initialization hasn't yet run (or if it won't run at all
because it's being used outside of a Rails app), and a gem tries to
subclass ViewComponent::Base, it would fail in Base.inherited due to
config.view_component_path being nil.

Fixes ViewComponent#1565
@donapieppo
Copy link
Contributor

donapieppo commented May 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants