active_job must be required

[aaronjensen]

See #330, which was closed but never fixed

The dependency was added in #331, but this was not sufficient. ActiveJob actually has to be required to be defined. Alternatively, the turbo job could only be declared if ActiveJob is defined.

Original text:

Because of https://github.com/hotwired/turbo-rails/blob/main/app/jobs/turbo/streams/broadcast_job.rb and https://github.com/hotwired/turbo-rails/blob/main/app/jobs/turbo/streams/action_broadcast_job.rb and the eager loader, active_job needs to be required by any app that is using turbo-rails.

Should turbo-rails somehow make these optional or require active_job explicitly?

A user who does not require active_job will see an uninitialized constant ActiveJob typically when they deploy to a production-like environment for the first time.

[dhh]

Let's add the explicit requirement. All the async broadcast_later stuff depends on it. I don't see that as optional.

[aaronjensen]

We use turbo rails without ever using broadcast_later (and we don't use active job at all). For us, it being optional would mean we could avoid the active job railtie entirely, which may not be a huge deal, but it's one more chunk of code we don't need not being requried.

The annoying part about making it optional is zeitwerk, because we can't just not declare the class. We would need to either move the class to lib and require it explicitly, or declare a dummy class which seems awful.

[dhh]

What's the gain you're getting from not having active job, even if you're not using it? If you're interesting in exploring it, I guess we can try something ala railties/all as the default, but allowing you to manually include what you need as well, if you load turbo-rails with require: false in your gemspec. Seems like a lot of work to me for a small gain, but.

[aaronjensen]

Could we check defined?(ActiveJob::Base) and do that automatically? The only caveat would be that that would make turbo rails need to be required after the active job railtie, but this tends to be expected and it's how Rails apps are generated.

The benefit is the same benefit we get from avoiding any code we don't need, and it has the potential of being more beneficial in the Rails ecosystem because we never know what is patched. I'm sure most (all?) patches are relegated to activesupport, but we try to avoid loading Rails code when we can in our applications. The general benefits are reduced startup time, reduced gem installation time, reduced package/image size, etc. One gem hardly matters for those, but when applied as a principle it adds up over time.

[jon-sully]

@aaronjensen I see in your PR that you followed the same pattern as the definition-check for ActionCable, so I wanted to share my experience and ask a couple of questions. The app I work on has been running turbo (drive and frames) for quite a while but I recently took to adding some real-time streams / broadcasting functionality. When I first attempted to get things working, I did see a background job failure in "ActionCable not defined". Before this point, we'd had the 'require actionable' line commented out in our config. I ended up just switching to rails/all there, but that's neither here nor there. I knew when that error popped up that the issue was "oh we don't actually require / use ActionCable so of course it's not defined" but I wonder if the best option here is to do both — have an initializer that only loads the jobs and channels if ActiveJob and ActionCable are defined, and setup some kind of better error message at runtime in the cases where those Jobs or Channels actually get invoked but ActiveJob and/or ActionCable isn't defined yet.

All that to say, not forcing the requirement of using ActiveJob and ActionCable, especially if you're not using those chunks of Turbo, feels good. But you never know if people are going to attempt to use those things anyway, so having great error messages that hint at "hey you might need to actually require those tools" may be great.

On the other hand, I don't necessarily see a net detriment in apps requiring ActiveJob, even if they don't use it directly. We use Sidekiq and invoke all of our Jobs directly to Sidekiq (include Sidekiq::Job + __.perform_async) but having ActiveJob also defined and configured (with one line in config.. config.active_job.queue_adapter = :sidekiq) means that any ActiveJob jobs will also 'just work'. So even though all of our own app's jobs are direct-to-Sidekiq jobs, other dependencies that look for ActiveJob will find it and have no issues invoking their jobs too. I think it's neat that you can have both jobs running through ActiveJob and jobs running over the native adapter, not a negative. Just my opinion though 😃

[aaronjensen]

@jon-sully

I knew when that error popped up that the issue was "oh we don't actually require / use ActionCable so of course it's not defined" but I wonder if the best option here is to do both...

You may not realize it, but you answered your own question here. It is readily apparent what the issue is when the error is encountered that is already in place. Adding additional complexity to introduce a slightly better error message doesn't seem worth it. It seems overly defensive, like adding a nil check in a method that expects to not receive nil. If a person does the wrong thing, an error happens and it doesn't take much to figure out what went wrong. Furthermore, if it were done here, shouldn't it be done everywhere else? Like when I access config.whatever.some_attribute? The inconsistency would be apparent.

On the other hand, I don't necessarily see a net detriment in apps requiring ActiveJob, even if they don't use it directly.

Ok, you don't see it, I can respect that. That doesn't mean there isn't. I'll try and explain it, but if you don't already see it, you still may not after my brief explanation. Rails isn't the most modular thing in the world. It's a massive bit of code, but it does, thankfully, have a handful of modules that can be opted in to or out of. This is baked into the template (it's what you encountered with some modules being commented out). Ultimately, it's not a matter of opinion, it's a matter of first-principles. We shouldn't deploy code we don't need, we shouldn't load code we don't need. This is especially true with Rails, which sees fit to monkey-patch core classes and actually change their behavior (see BigDecimal#to_s for example). While most of those changes are relegated to activesupport, which is required by every other Rails package, it is our responsibility to limit the blast-radius of things that have shown a tendency to skirt fundamentals.

I think it's neat that you can have both jobs running through ActiveJob and jobs running over the native adapter, not a negative.

Well, you're not on my team, but I will just say that our team explicitly avoids allowing anything that is "neat" or "tickles us" influence our decisions about things. We consider that an early warning sign that we are about to make the wrong decision. Having two completely different ways of doing things co-exist is an invitation to unnecessary variation, which is an invitation to slowing down as a team. Again, these points are rather self-evident, so if they are not already recognized by someone they likely won't be after this response.

[jon-sully]

😆 I can respect these points for sure. And while I understood the error message immediately (and you obviously would too), that doesn't mean a less experienced person would. Of course, you're not wrong:

like adding a nil check in a method that expects to not receive nil

And I agree; just putting the idea out.

Surely I'm not going to argue that ActiveSupport does a lot of things or that ActiveJob in it of itself is a large library, I suppose I'm red-teaming the idea presented in this issue and your thoughts check out (to me) 👍

To get back on point with the issue and your PR, and to close my thoughts in short, I like it 🙂 Thanks for humoring me!

[aaronjensen]

Sounds good. One last bit:

...that doesn't mean a less experienced person would.

Consider this: by adding the additional "friendly" error message, we may actually be robbing people of relatively safe opportunities to learn important things about how to do their job. You benefited from it being obvious to you what to do when you encountered that error. Developers that are more junior would as well, but they have to be given the opportunity to learn and practice those skills.

Also, thank you for bringing it back to my attention that my PR is still lingering.

@dhh would you be able to merge #479 when you get a chance please? It follows suit with the same precedent established for ActionCable and I just rebased it so it should be ready to go. Thank you.