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

Fix lazy-loading of ActiveRecord #1281

Merged
merged 3 commits into from
Mar 29, 2021
Merged

Fix lazy-loading of ActiveRecord #1281

merged 3 commits into from
Mar 29, 2021

Conversation

jaredbeck
Copy link
Member

Superesedes #1237, preserving Eric Proulx' authorship.

I've fixed some of my own concerns from #1237, but two remain:

  1. Most people have some PaperTrail configuration in an initializer of their own, config/initializers/paper_trail.rb. The documentation encourages this. I assume we want the end-user's initializer to happen last? If so, is there a way to make that explicit in code?
  2. What is the best way to test these changes?

@jaredbeck jaredbeck marked this pull request as draft December 27, 2020 05:40
@jaredbeck jaredbeck mentioned this pull request Dec 27, 2020
@jaredbeck jaredbeck marked this pull request as ready for review March 18, 2021 03:03
ericproulx and others added 3 commits March 17, 2021 23:42
An effort begun by Eric Proulx in
#1237

- Replaces Engine with simpler Railtie
- Critically, defers certain `requires`
  - With Rails, defers via a Lazy Load Hook in the Railtie
  - Without Rails, just by require-ing AR first
- Ensure that the "paper_trail" initializer happens before the
end-user's initializers (in their app's config/initializers)
- Document the boot process of dummy_app and how it differs
from a conventional app.

This is a direct continuation of fc6c5f6, which was a collaboration
between Eric and myself, but I choose to make this a separate
commit, partly for vanity, and partly on the faint hope that it
might make review easier.
Hopefully this will prevent people from trying to add
rails-controller-testing to our gemspec.
@jaredbeck
Copy link
Member Author

.. I assume we want the end-user's initializer to happen last?

I was able to make this explicit by adding the following:

- initializer "paper_trail" do
+ initializer "paper_trail", before: "load_config_initializers" do

Incidentally, to my delight, it seems that Rails initializers are not a simple queue, but rather a directed, acyclic graph. See the tsort_each in Rails::Initializable#run_initializers if you're curious.

What is the best way to test these changes?

It's difficult to test these changes with our spec/dummy_app because its boot process does not perfectly match a conventional Rails app. To gain the necessary confidence, we can test this branch in our real Rails apps, with the usual technique:

# Gemfile
gem 'paper_trail', github: 'paper-trail-gem/paper_trail', branch: 'jb_lazy_load_rails'

I have tested this branch in this way, and it'd be great if at least one other person could too.

So, I think this is ready for final review. @ericproulx of course, and who else? @seanlinsley ? @batter ?

@jaredbeck
Copy link
Member Author

Our next release will most likely be a major version, 12, because master already contains breaking changes. This is sort of a breaking change also, so I'd like to include it in 12. It's the last thing that I'd like to include in 12. If any committers have other requests, please email me.

@tlynam
Copy link
Contributor

tlynam commented Mar 22, 2021

I can help test this locally

@jaredbeck
Copy link
Member Author

I can help test this locally

Thanks Todd, how'd it go?

@tlynam
Copy link
Contributor

tlynam commented Mar 25, 2021

Sorry @jaredbeck, I should have mentioned I was planning on testing this weekend

@tlynam
Copy link
Contributor

tlynam commented Mar 28, 2021

Hi @jaredbeck, this looks good to me. Tested:

  • PaperTrail.enabled = false in initializer
  • whodunnit in controller
  • some model configs
  • reify

Copy link
Contributor

@tlynam tlynam left a comment

Choose a reason for hiding this comment

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

LGTM!

@jaredbeck jaredbeck merged commit bee9213 into master Mar 29, 2021
@jaredbeck jaredbeck deleted the jb_lazy_load_rails branch March 29, 2021 14:54
@jaredbeck
Copy link
Member Author

Thank you @ericproulx for writing the heart of this, and @tlynam for testing.

I'll release this shortly as 12.0.0.

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.

3 participants