[Fix Regression] Fix Broadcastable inclusion in new Rails apps #678
Conversation
|
@dhh Sorry to nag, but I'm not seeing any comments on here. I can address them as soon as I see them so we can get this merged and released. |
| ActiveSupport.on_load(:active_job) do | ||
| ActiveSupport.on_load(:active_record) do | ||
| ActiveSupport.on_load(:active_record) do | ||
| if defined?(ActiveJob) |
There was a problem hiding this comment.
Why won't this have the same potential timing issue? If active job isn't loaded before active record, this will fail. That can happen if folks are manually managing their load order.
There was a problem hiding this comment.
There would still be a load order issue if someone loaded turbo-rails, then activated ActiveRecord and then loaded ActiveJob. The problem before was that ActiveJob would not get activated (and the ActiveSupport on_load) would not get triggered until after one of the broadcast macros was invoked. Because require "active_job" defines ActiveJob immediately, this works around it. I changed the order so that it wouldn't check until after ActiveRecord was activated. This means someone should be able to require turbo-rails prior to active job/active record and it'd still work assuming active record isn't activated.
There was a problem hiding this comment.
Let me double check my assumption about active record's on_load
There was a problem hiding this comment.
Actually yeah, so in order for this to be a problem, they would actually have to not require active_job until after Rails was initialized. I'm pretty sure all sorts of things would go sideways if they did that, so I don't know that that's necessary to account for.
If you'd like to get this merged and discuss other ways to approach this, I'd be happy to.
There was a problem hiding this comment.
Okay, we'll get this out as a rush fix for now, and can then review.
There was a problem hiding this comment.
Sounds good, thanks. I just added you on LinkedIn, ping me there if you want to have a quick call about it, may be easier than GitHub back and forth.
| @@ -1,269 +1,263 @@ | |||
| require "test_helper" | |||
|
|
|||
| module Turbo | |||
There was a problem hiding this comment.
This seems like a bunch of unrelated changes by an auto styler?
There was a problem hiding this comment.
This is undoing the workaround I had to do in the original PR because it's no longer needed after this change. It should now be consistent with the other tests and what was in main prior to my original PR.
|
Comments were pending as part of a review apparently, sorry about that. |
No worries -- that's a pretty bad GitHub UX SNAFU, but what can we do. |
Fixes a serious regression caused by #602
The problem is that in new Rails apps, ActiveJob may never be loaded before a
broadcast_*invocation is made. If that happens, thebroadcast_*invocation will fail with a method missing.Instead of waiting for ActiveJob to trigger
on_load, the code now checks for the existence of theActiveJobmodule. This should be more resilient.This should be merged and released ASAP, as the regression likely affects many.