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.

Sign up for GitHub

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

[Fix Regression] Fix Broadcastable inclusion in new Rails apps #678

Merged
merged 1 commit into from
Sep 18, 2024
Merged

[Fix Regression] Fix Broadcastable inclusion in new Rails apps #678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/turbo/engine.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ class Engine < Rails::Engine
end

initializer "turbo.broadcastable" do
ActiveSupport.on_load(:active_job) do
ActiveSupport.on_load(:active_record) do
ActiveSupport.on_load(:active_record) do
if defined?(ActiveJob)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me double check my assumption about active record's on_load

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, we'll get this out as a rush fix for now, and can then review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

include Turbo::Broadcastable
end
end
Expand Down Expand Up @@ -123,9 +123,9 @@ class Engine < Rails::Engine
include Turbo::TestAssertions
end

ActiveSupport.on_load(:active_job) do
ActiveSupport.on_load(:action_cable) do
ActiveSupport.on_load(:active_support_test_case) do
ActiveSupport.on_load(:action_cable) do
ActiveSupport.on_load(:active_support_test_case) do
if defined?(ActiveJob)
require "turbo/broadcastable/test_helper"
include Turbo::Broadcastable::TestHelper
end
Expand Down
Loading
Loading