search

bootstrap-ruby / bootstrap_form

Official repository of the bootstrap_form gem, a Rails form builder that makes it super easy to create beautiful-looking forms using Bootstrap 5.
MIT License
1.64k stars 352 forks source link

Lazy load ActionText helpers #719

Closed jdufresne closed 7 months ago

jdufresne commented 7 months ago

My application doesn't use ActionText.

Either way, Rails bootstrap_form loads it at:

https://github.com/bootstrap-ruby/bootstrap_form/blob/ed3eda885c2145a364e8affd759f42b03ff6ee7c/lib/bootstrap_form.rb#L1-L9

This require statement creates the ActionText module which "tricks" other gems into loading their ActionText support. These gems may do something like:

if defined?(ActionText)
  # Load ActionText support ...
end

This can result in a confusing error message later on when this loaded support eventually fails.

One example of this is RailsAdmin:

https://github.com/railsadminteam/rails_admin/blob/13c90fdd95c3755035b44264dc61e4b65c09ff12/app/assets/javascripts/rails_admin/application.js.erb#L27-L30

I'm wondering if bootstrap_form could somehow use lazy loading to prevent creating the global ActionText module until necessary. If possible, this would improve compatibility between these gems when ActionText is not used.

lcreid commented 7 months ago

Thanks for reporting this, and thanks for doing some digging to start us on a solution. We had another PR that attempted to fix this but obviously it wasn't enough. If you want to take a stab at a PR yourself, say so in a comment. Otherwise we'll try to do this better in the near future.

lcreid commented 7 months ago

Hi, @jdufresne . I think I'm reproducing this, but I want to make sure I understand: the application doesn't raise any errors or anything, but it loads ActionText which is code bloat in itself, and may cause issues with other gems, etc. I'm not trying to minimize your concern. I think you're right. I just want to make sure that I'm solving the right issue.

jdufresne commented 7 months ago

It is not simply bloat. The application does raise an exception as a combination of these two gems using different approaches to lazy loading ActionText.

For me, this exception comes when compiling static assets using sprockets:

$ bundle exec rails assets:precompile
bin/rails aborted!
Sprockets::FileNotFound: couldn't find file 'trix' with type 'application/javascript' (Sprockets::FileNotFound)
Checked in these paths:
  .../app/assets/config
  .../app/assets/images
  .../vendor/bundle/ruby/3.2.0/gems/rails_admin-3.1.2/app/assets/javascripts
  .../vendor/bundle/ruby/3.2.0/gems/rails_admin-3.1.2/app/assets/stylesheets
  .../vendor/bundle/ruby/3.2.0/gems/rails_admin-3.1.2/vendor/assets/fonts
  .../vendor/bundle/ruby/3.2.0/gems/rails_admin-3.1.2/vendor/assets/javascripts
  .../vendor/bundle/ruby/3.2.0/gems/rails_admin-3.1.2/vendor/assets/stylesheets
  .../vendor/bundle/ruby/3.2.0/gems/turbo-rails-1.5.0/app/assets/javascripts
  .../vendor/bundle/ruby/3.2.0/gems/nested_form-0.3.2/vendor/assets/javascripts
  .../vendor/bundle/ruby/3.2.0/gems/bootstrap_form-5.4.0/app/assets/stylesheets
  .../vendor/bundle/ruby/3.2.0/gems/actioncable-7.1.2/app/assets/javascripts
  .../vendor/bundle/ruby/3.2.0/gems/activestorage-7.1.2/app/assets/javascripts
  .../vendor/bundle/ruby/3.2.0/gems/actionview-7.1.2/app/assets/javascripts
  .../vendor/bundle/ruby/3.2.0/gems/actionview-7.1.2/lib/assets/compiled
  .../vendor/bundle/ruby/3.2.0/gems/rails_admin-3.1.2/src
.../vendor/bundle/ruby/3.2.0/gems/rails_admin-3.1.2/app/assets/javascripts/rails_admin/application.js.erb:24

As you can see in the stack trace, the error occurs not in bootstrap_form, but RailsAdmin. However, the reason for this is bootstrap_form eagerly loads the ActionText module. The existence of this module "tricks" RailsAdmin into compiling ActionText assets which shouldn't happen and we get the exception above.

I have also filed an issue with RailsAdmin as I believe both projects could be improved:

  1. bootstrap_form shouldn't eagerly include the ActionText module (this issue)
  2. RailsAdmin could have better ActionText detection than defined?(ActionText) (https://github.com/railsadminteam/rails_admin/issues/3659)
jdufresne commented 7 months ago

Would it help if I created a minimal test project to demonstrate the issue? If so, I can work towards that and post here.

lcreid commented 7 months ago

Yeah, if it's not too much trouble it would help to have an example that demonstrates the issue. Thanks for clarifying what's happening.

jdufresne commented 7 months ago

Here is an example:

https://github.com/jdufresne/rails-example-action-text-issue

To experience the error, run:

bundle install
bundle exec rails assets:precompile

Commenting out bootstrap_form's require "#{Gem::Specification.find_by_name('actiontext').gem_dir}/app/helpers/action_text/tag_helper" makes the error go away.

Let me know if you have any other questions.

jdufresne commented 7 months ago

bootstrap_form has the following comment in the code (emphasis mine):

NOTE: The rich_text_area and rich_text_area_tag helpers are defined in a file with a different name and not in the usual autoload-reachable way. The following line is definitely need to make bootstrap_form work.

Do you have more information about this?

I'm now wondering why bootstrap_form needs this require at all and can't instead rely on the Rail's engine/railtie system to pull in the helper when ActionText is used.

jdufresne commented 7 months ago

bootstrap_form has the following comment in the code (emphasis mine):

NOTE: The rich_text_area and rich_text_area_tag helpers are defined in a file with a different name and not in the usual autoload-reachable way. The following line is definitely need to make bootstrap_form work.

Do you have more information about this?

I'm now wondering why bootstrap_form needs this require at all and can't instead rely on the Rail's engine/railtie system to pull in the helper when ActionText is used.

I've tested this idea in https://github.com/bootstrap-ruby/bootstrap_form/pull/720. FWIW, CI passes.

lcreid commented 7 months ago

I'm now wondering why bootstrap_form needs this require at all and can't instead rely on the Rail's engine/railtie system to pull in the helper when ActionText is used.

This is why I hadn't made any progress. I was exploring the proper way to do it. More exactly, I was exploring how best to test the gem. We've had some issues over the years because our tests don't run in a real Rails environment. I'll take a look at your PR.

jdufresne commented 3 weeks ago

@lcreid WDYT about cutting a new release that includes this fix? I'm looking to remove ActionText from my apps. Thanks!