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

Lazy-load rails interactions #456

Merged
merged 4 commits into from
May 6, 2023

Conversation

ngan
Copy link
Contributor

@ngan ngan commented Mar 5, 2023

Thus PR does a few things:

  • Rename lib/rails.rb to lib/railtie.rb to follow modern conventions.
  • Register a Railtie and add an arbre initializer that listens for action_view loading. Only then do we want to register the template handler.
  • Change all send :include to just include since this gem supports only version of Ruby where it’s a public method.

The way it's current done is problematic for me because it attempts to reference ActionView before it's loaded by the application itself (config/application.rb). Requiring the rails gem doesn't actually load all the rails gems.

@javierjulio
Copy link
Member

Surprising this hasn't come up before. Thanks! Tests are running now.

@ngan
Copy link
Contributor Author

ngan commented Mar 5, 2023

Hey @javierjulio I think I fixed the build. This problem surfaced when I tried to use sidekiqswarm which I guess preloads all the gems without loading the rails application.

I think the next version of Rails is going to be strict about gems that are inadvertently loading Rails concerns without using load hooks: rails/rails#46047

@ngan
Copy link
Contributor Author

ngan commented Mar 6, 2023

@javierjulio @deivid-rodriguez please let me know if I need to do anything else to get this merged and released. Happy to help!

@ngan
Copy link
Contributor Author

ngan commented Mar 8, 2023

@javierjulio @deivid-rodriguez just wanted to kindly bump this 🙇

@javierjulio
Copy link
Member

@ngan I will get to this soon, thank you.

@javierjulio javierjulio force-pushed the lazy-load-rails-concerns branch 2 times, most recently from 9f3a4a4 to 0f43111 Compare March 14, 2023 16:46
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.29 🎉

Comparison is base (83107dc) 91.36% compared to head (250063d) 91.66%.

❗ Current head 250063d differs from pull request most recent head 5478006. Consider uploading reports for the commit 5478006 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
+ Coverage   91.36%   91.66%   +0.29%     
==========================================
  Files          17       17              
  Lines         475      480       +5     
==========================================
+ Hits          434      440       +6     
+ Misses         41       40       -1     
Impacted Files Coverage Δ
lib/arbre/rails/rendering.rb 87.50% <ø> (ø)
lib/arbre/rails/template_handler.rb 100.00% <ø> (ø)
lib/arbre.rb 100.00% <100.00%> (+6.66%) ⬆️
lib/arbre/railtie.rb 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@javierjulio javierjulio force-pushed the lazy-load-rails-concerns branch from 0f43111 to a19592e Compare March 19, 2023 00:04
@javierjulio javierjulio force-pushed the lazy-load-rails-concerns branch from a19592e to 7d4ae0f Compare March 19, 2023 01:50
@ngan
Copy link
Contributor Author

ngan commented Apr 13, 2023

@javierjulio this is ready for another look! 🙇

@javierjulio javierjulio force-pushed the lazy-load-rails-concerns branch from 250063d to 5478006 Compare May 6, 2023 00:21
Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thanks!

@javierjulio javierjulio merged commit ae2bfac into activeadmin:master May 6, 2023
@ngan ngan deleted the lazy-load-rails-concerns branch May 6, 2023 01:12
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.

2 participants