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

Determine NamespacedClassFinder#finder method after rails config #1336

Merged
merged 3 commits into from
Jun 1, 2021

Conversation

s-mage
Copy link
Contributor

@s-mage s-mage commented Mar 12, 2021

Hi there!

You're making a great gem, thank you very much.

I've found a bug while profiling a thing in our app.

Formtastic has two modes of finding classes, one is fast and another fetches new files in development. It works in dev mode all the time because use_const_defined? is called when Rails.application is still nil.

As a result, finding input classes sometimes takes half of the time Formtastic works.

This PR fixes this by determining the finder method on the first calling, which likely happens after the app initialization.

Bonus: spec for the class tested changed cache_classes config value, which, I suppose, was used before eager_load.

Testing

I don't know how to make a spec for the case, but here is how I do it manually:

  1. config.eager_load = !!ENV["EAGER_LOAD"] # config/environments/development.rb
  2. EAGER_LOAD=true rails c
  3. For master:
show-source Formtastic::NamespacedClassFinder#finder # for pry
Formtastic::NamespacedClassFinder.new([]).method(:finder).source_location # would work everywhere

For this PR:

Formtastic::NamespacedClassFinder.send(:finder_method)

Workaround

Just in case the fix won't get to master or will be released only for v4. I've added a workaround to reach the same effect

# config/initializers/formtastic_namespaced_class_finder_patch.rb
if Rails.configuration.eager_load
  class Formtastic::NamespacedClassFinder
    private
      def finder(class_name)
        find_with_const_defined(class_name)
      end
  end
end

Before this patch it used find_by_trying all the time, because class is
required before rails config.
Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

Makes sense and it looks good. The failing tests were fixed in a different PR. Thanks for this!

@mikz mikz merged commit 3bff5cb into formtastic:master Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants