Do not load ActiveJobs if ActiveJob is not loaded

[aaronjensen]

Fixes #478

Replaces #479

Based on #601

[zzak]

Would this work instead?

ActiveSupport.on_load :active_job do

[aaronjensen]

I don't know. I'm following the pattern used in the Action Cable check below. At first blush, it seems preferable to the method used here, but it would need to be tested.

[zzak]

Correct, sorry I was thinking of the opposite case but this initializer occurs in the absence of AJ 🙏

[NakagawaMakoto]

I really do want this PR to be merged.

I have created my project with the following;

rails new myproj --skip-action-mailer --skip-action-mailbox --skip-action-text --skip-active-record --skip-active-job --skip-active-storage --skip-action-cable --skip-jbuilder --skip-system-test --skip-rubocop --skip-ci --skip-devcontainer --skip-git --skip-docker --css sass --asset-pipeline=propshaft

It works fine in development/test mode, but fails to start with the following in production mode with eager_load set to true;

c:/ruby33-x64/lib/ruby/gems/3.3.0/gems/turbo-rails-2.0.6/app/jobs/turbo/streams/action_broadcast_job.rb:2:in `<main>': uninitialized constant ActiveJob (NameError)

I do not really think my project gain much by setting eager_load to true, but it would be better not to be bothered by ActiveJob when you know you explicitly exclude the feature.

[dhh]

I can't see how this is going to work. Then all the "broadcast_later" calls are just going to fail at runtime. I appreciate wanting a minimal version of Rails for whatever reasons, but this turbo-rails integration is premised on the full integration with broadcasts and what not. I think you're probably better off just pulling in Turbo as a JavaScript dependency and forgoing this integration gem all together.,

[aaronjensen]

@dhh It'd work because someone who wasn't using ActiveJob also wouldn't be using broadcast_later. The gem itself is useful to people that aren't because of the stream related helpers. Ultimately, this is a cohesion problem. One could, as you said, use the JavaScript dependency directly and implement their own turbo helpers (or copy the relevant ones from this package).

The idea with this was to make this package work for anyone to use the way they wanted to use it. Rails itself is modular enough to allow opting out of ActiveJob. I thought it made sense for other packages that are bundled with Rails to respect the modularity decisions of the users. If someone ends up with a runtime error, they end up with a runtime error. They'd discover that well before they went to production and add the active_job dependency.

Ideally the "later" versions of methods wouldn't be available at all, so the broadcast_later macro would fail at eval time.

[dhh]

Do explore it further. But I can't take something where methods are added that then just don't work. It has to be cohesive, as you say. What's there, what's added, is going to work.

[aaronjensen]

Would you be amenable to the Broadcastable concern not being mixed in to the model at all if ActiveJob is unavailable?

[dhh]

Something like that, but it has to be cohesive. Across the line, we can't have code that's dead at runtime, if we allow this to work without activejob. And the complexity of introducing that has to be proportionate to the fact that this is a minority concern and that the vast majority of users will not do this.

[aaronjensen]

@dhh see the updated PR. I exclude Broadcastable and its associated test helper. I also added a note to the documentation within Broadcastable. As far as I can tell, this is cohesive. If you opt out of ActiveJob, you won't get Broadcastable or any of its associated jobs. If you include ActiveJob, you will.

[aaronjensen]

One moment, fixing what seems like a local test running issue.

[aaronjensen]

This is necessary because Turbo::Broadcastable::TestHelper isn't loaded and therefore the namespace isn't available until after the include Turbo::Broadcastable::TestHelper, due to lazy loading. We could use if defined?(ActiveJob), rather than ActiveSupport.on_load(:active_job), but that doesn't seem better.

[aaronjensen]

Fixed. One caveat: https://github.com/hotwired/turbo-rails/pull/602/files?w=1#r1760143885

[dhh]

That works 👍

[aaronjensen]

Great, thank you!

[botandrose]

@aaronjensen @dhh I think there's still more to do here. Bumping from v2.0.6. to v2.0.8 broke my app because Broadcastable is no longer included, even though I have require "rails/all" in my config/application.rb. Trying to narrow down to a failing test.

[aaronjensen]

@botandrose in the interim could you post the stack trace? I've tried a few things myself and haven't seen any failures.

[botandrose]

Hi @aaronjensen, thanks for the quick response.

My stack trace is just a method missing error for one the broadcast methods, so its not particularly helpful, but I have just confirmed its a regression, and not something weird I'm doing in my app:

Here is fresh Rails 7.2.1 app with zero changes from rails new, and you can see that ApplicationRecord is missing Turbo::Broadcastable. Downgrading to turbo-rails from 2.0.8 to 2.0.7 brings the module back:

$ rvm use ruby-3.3.4@turbo-rails
Using /home/micah/.rvm/gems/ruby-3.3.4 with gemset turbo-rails

$ rails new bug-turbo-rails && cd bug-turbo-rails
<snip>

$ bin/rails runner 'puts ApplicationRecord.included_modules.include?(Turbo::Broadcastable)'
false

$ vi Gemfile # pin turbo-rails to 2.0.7

$ bundle update turbo-rails
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Resolving dependencies...
Fetching turbo-rails 2.0.7 (was 2.0.8)
Installing turbo-rails 2.0.7 (was 2.0.8)
Note: turbo-rails version regressed from 2.0.8 to 2.0.7
Bundle updated!

$ bin/rails runner 'puts ApplicationRecord.included_modules.include?(Turbo::Broadcastable)'
true

I'm not sure why the tests aren't catching this, but it seems clear that its a regression for the common case.

[aaronjensen]

That's not good. I'm sorry about that. Looking now.

[dhh]

@aaronjensen Let me know if we need to revert for an emergency rerelease. I'd say if we can't pinpoint the problem this morning, that's what we need to do.

[aaronjensen]

@dhh pinpointed, PR available: #678

[aaronjensen]

@botandrose if you'd be able change the initializer "turbo.broadcastable" in your locally installed gems' lib/turbo/engine.rb to the below and test, that would be appreciated.

    initializer "turbo.broadcastable" do
      ActiveSupport.on_load(:active_record) do
        if defined?(ActiveJob)
          include Turbo::Broadcastable
        end
      end
    end

[botandrose]

@aaronjensen Confirmed that this change fixes the issue in both the fresh rails app and in my specific app!

[aaronjensen]

@botandrose Thank you for confirming. @dhh #678 should be ready to merge and release. I'm standing by if anything else needs to be adjusted.

[dhh]

@aaronjensen Thanks! I have a few comments on that PR.

[aaronjensen]

@dhh Great. FYI I don't see them yet, not sure if you are still working on them.

Maybe they're batched in GitHub's (problematic) comment batching UI?

[dhh]

Released fix in https://github.com/hotwired/turbo-rails/releases/tag/v2.0.9

[botandrose]

Thanks for the quick turnaround, guys!