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

Upgrade dependencies and add support for Rails 7.2 and 8.0 #37

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

Conversation

rzane
Copy link

@rzane rzane commented Jan 21, 2025

Summary

Provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

  • Changes required_ruby_version to >= 3.2
  • Changes Rails version support to >= 7.0, < 8.1
  • Removes unnecessary files and configuration from spec/dummy
  • Removes unnecessary >= 5.2 checks
  • Commits lockfiles, since that's what we do nowadays
  • Runs CI against Ruby 3.2, 3.3, and 3.4

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.

Thanks for contributing to Journaled!

/domain @Betterment/journaled-owners
/platform @jmileham @coreyja @ceslami @danf1024

@rzane rzane force-pushed the rzane/upgrade-deps branch from 43e9f7e to d40d755 Compare January 21, 2025 21:32
config.active_support.deprecation = :raise

if ActiveJob.gem_version < Gem::Version.new('8.0')
config.active_job.enqueue_after_transaction_commit = :never
Copy link
Member

Choose a reason for hiding this comment

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

I'm stale on some of this stuff. I thought this was added in rails 8. But am I reading this correctly? We want to only add this in rails less than 8?

Copy link
Author

Choose a reason for hiding this comment

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

This is really complicated and confusing. This spec was failing Rails 7.2, but passing for all other versions (including 8.0).

A feature was added in Rails 7.2 that would enqueue jobs after the transaction commits. There were three possible configurations for this flag (see here):

  • :default - Defer to the adapter's #enqueue_after_transaction_commit? method
  • :never - Don't wait until after commit.
  • :always - Always wait until after commit.

In our test suite, we use the :test adapter, which returns true from enqueue_after_transaction_commit?. Delayed, for example, returns false.

In Rails 8.0, they decided this was actually a pretty dangerous configuration. They stopped delegating to the adapter -- :default is now the same as :never.

So, in summary, we'll set this to :never to make 7.2 behave the same as all other versions.

Copy link
Author

Choose a reason for hiding this comment

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

I made a change to explain why this exists and make sure it doesn't apply to versions < 7.2.

Copy link
Author

Choose a reason for hiding this comment

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

We could also add a runtime check to ensure that ActiveJob is not configured to enqueue after commit, but it would be complicated.

module Journaled
  if ActiveJob.gem_version >= Gem::Version.new('8.0')
    def self.enqueue_after_transaction_commit?
      job_base_class_name.constantize.enqueue_after_transaction_commit.in?([true, :always])
    end
  elsif ActiveJob.gem_version >= Gem::Version.new('7.2')
    def self.enqueue_after_transaction_commit?
      job_base_class = job_base_class_name.constantize

      case job_base_class.enqueue_after_transaction_commit
      when :default
        job_base_class.queue_adapter.enqueue_after_transaction_commit?
      when :always
        true
      else
        false
      end
    end
  else
    def self.enqueue_after_transaction_commit?
      false
    end
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow. Thanks for explaining. Also, it's nice to know I'm not entirely out of the loop. Glad I asked.

Copy link
Member

Choose a reason for hiding this comment

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

I defer to @smudge on whether we need a runtime check for this.

My gut is that we're okay without it. But he has a far better grasp of the risks.

@rzane rzane requested a review from samandmoore January 22, 2025 15:31
Copy link
Member

@samandmoore samandmoore left a comment

Choose a reason for hiding this comment

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

domainlgtm

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