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

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

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

aaronjensen
Copy link
Contributor

@aaronjensen aaronjensen commented Sep 18, 2024

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, the broadcast_* invocation will fail with a method missing.

Instead of waiting for ActiveJob to trigger on_load, the code now checks for the existence of the ActiveJob module. This should be more resilient.

This should be merged and released ASAP, as the regression likely affects many.

@aaronjensen
Copy link
Contributor Author

@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)
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.

@@ -1,269 +1,263 @@
require "test_helper"

module Turbo
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a bunch of unrelated changes by an auto styler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@dhh
Copy link
Member

dhh commented Sep 18, 2024

Comments were pending as part of a review apparently, sorry about that.

@aaronjensen
Copy link
Contributor Author

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.

@dhh dhh merged commit 3c3bafa into hotwired:main Sep 18, 2024
15 checks passed
@dhh
Copy link
Member

dhh commented Sep 18, 2024

@aaronjensen aaronjensen deleted the broadcastable-include-fix branch September 18, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants