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

Minor Rails 6.1 fix, plus autoload Delayed::Job constant #2

Merged
merged 4 commits into from
Sep 2, 2021

Conversation

smudge
Copy link
Member

@smudge smudge commented Aug 28, 2021

/domain @samandmoore @effron
/no-platform

This fixes an issue I was seeing on a very slim Rails 6.1 app -- essentially, ActiveSupport.on_load(:active_record) was not triggering at all during rake delayed:work, resulting in a missing constant error. Previous versions of rails seem to load ActiveRecord::Base during boot, but 6.1 seems to have found a way to optimize that step away.

This PR moves the delayed/job.rb class to app/models and converts the gem's Railtie to an Engine, allowing it to autoload files in app/models by convention. In cases where we are not in a Rails app (and Rails::Engine is not defined), we fall back on loading active_record manually and then running a require_relative of the specific job.rb file. I based this on other "rails-engine-optional" gems I've encountered, and confirmed that it works in an irb console.

I also removed require 'delayed/serialization/active_record' because it would already be loaded inside of require 'delayed/yaml_ext'. It also only applies to syck which dates back to Ruby 1.9, and we should consider removing support for it entirely. (It can still be installed via the syck gem, but psych has been the default for many, many years.)

@nanda-prbot
Copy link

Needs somebody from @samandmoore and @effron to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

HOW TO: Claim a Review

@nanda-prbot
Copy link

Needs somebody from @samandmoore and @effron to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

HOW TO: Claim a Review

1 similar comment
@nanda-prbot
Copy link

Needs somebody from @samandmoore and @effron to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

HOW TO: Claim a Review

@samandmoore
Copy link
Member

<<domainlgtm

This seems a lil odd to me but it also seems fine. I don't have any better ideas, just ones that are weird in different ways lol

nanda-prbot
nanda-prbot previously approved these changes Aug 30, 2021
Copy link

@nanda-prbot nanda-prbot left a comment

Choose a reason for hiding this comment

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

Approved! ⚡ 🌟 🎯

@nanda-prbot
Copy link

This PR requires additional review because of new changes

Please get another domain review from @samandmoore, or another reviewer with write access if unavailable.

@smudge
Copy link
Member Author

smudge commented Aug 30, 2021

bump @samandmoore -- ended up updating version+changelog in this PR

@samandmoore
Copy link
Member

domainlgtm

Copy link

@nanda-prbot nanda-prbot left a comment

Choose a reason for hiding this comment

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

Approved! 🌮 🍟 🍕

@smudge smudge merged commit 484d68a into Betterment:main Sep 2, 2021
@smudge smudge deleted the engine-autoload-model branch September 2, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants