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

Move ActiveJob adapter to delayed_job #1219

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dixpac
Copy link

@dixpac dixpac commented Sep 16, 2024

Implements adapter for ActiveJob inside the gem. This adapter is defined only if rails version is >=8, for earlier rails versions adapter is defined inside the Rails itself.

I'm not supper familiar with delayed_job internals, this is just a draft if you are interested in doing this I can finish the work on this PR.
This is connected to the discussion here rails/rails#52929

❤️

Implements adapter for `ActiveJob` inside the gem.
This adapter is defined only if rails version is >=8,
for earlier rails versions adapter is definied inside the Rails itself
@dixpac dixpac force-pushed the dix/add_active_job_adapter branch from acf1951 to 8fd6420 Compare October 13, 2024 16:39
@dixpac dixpac force-pushed the dix/add_active_job_adapter branch from 8fd6420 to 41f2b77 Compare October 13, 2024 16:40
@dixpac dixpac marked this pull request as ready for review October 13, 2024 16:40
@dixpac
Copy link
Author

dixpac commented Oct 13, 2024

@albus522 I've moved adapter from Rails and added the tests. Let me know what do you think about this approach

module ActiveJob
module QueueAdapters
# Explicitly remove the implementation existing in older rails'.
remove_const(:DelayedJobAdapter) if defined?(:DelayedJobAdapter)
Copy link
Member

Choose a reason for hiding this comment

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

Since the adapter signature has changed over time, I think this file needs to be the fallback for when it is not present in ActiveJob.

Copy link
Author

@dixpac dixpac Nov 16, 2024

Choose a reason for hiding this comment

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

We can load this adapter only if ActiveJob.version > Gem::Version.new("8.0").
Afaik plan is to "deprecate" adapter in rails for 8.1 and remove it in later Rails versions in favour of the adapter inside the dj. Wdyt?

@@ -1,5 +1,6 @@
require 'delayed_job'
require 'rails'
require 'active_job/queue_adapters/delayed_job_adapter'
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be in lib/delayed_job.rb using a similar ActiveSupport.on_load pattern as action mailer.

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