Skip to content

conditionally require graphql/dashboard#5515

Closed
chaadow wants to merge 1 commit intormosolgo:masterfrom
chaadow:patch-1
Closed

conditionally require graphql/dashboard#5515
chaadow wants to merge 1 commit intormosolgo:masterfrom
chaadow:patch-1

Conversation

@chaadow
Copy link

@chaadow chaadow commented Jan 25, 2026

More context here: https://github.com/rmosolgo/graphql-ruby/pull/5505/files#r2725942442

Using this script, this require has triggered a lot of autoloaded constants related to ActionController
image

@hotoolong
Copy link

Thanks for working on this fix! This change will resolve the immediate issue we encountered with rambulance (and likely other gems using on_load(:action_controller)).

However, I have a concern about using on_load(:action_controller) as the solution:

Potential Issue

With on_load(:action_controller), if another gem loads ActionController early (before Zeitwerk is set up), the same problem could reoccur:

  1. Some gem calls require 'action_controller' during Bundler's gem loading phase
  2. ActionController::Base loads → run_load_hooks(:action_controller) fires
  3. graphql's on_load callback executes → require 'graphql/dashboard'
  4. graphql/dashboard.rb defines class ApplicationController < ActionController::Base
  5. inherited hook fires → helper :allNameError: uninitialized constant ApplicationHelper

This is essentially the same issue, just triggered by a different gem instead of graphql itself.

Suggested Alternative

Using a Railtie initializer would be more robust, as it allows explicit control over the execution timing within Rails' initialization sequence:

# graphql.rb
if defined?(::Rails::Engine)
  autoload :Dashboard, 'graphql/dashboard'
end

# graphql/railtie.rb
initializer 'graphql.setup_dashboard', before: :add_routing_paths do
  require 'graphql/dashboard'
end

This ensures the Dashboard is loaded at a specific point in Rails' initialization (after Zeitwerk is configured), regardless of when other gems might load ActionController.

That said, I understand this change is simpler and solves the immediate problem. Just wanted to flag this potential edge case for consideration.

@rmosolgo
Copy link
Owner

rmosolgo commented Feb 3, 2026

👋 I'm definitely interested in fixing this, but I don't know how to replicate the problem. Could one of you share a script that demonstrates the problem? For example, some rails console command that you run, rake task, or puts messages -- anything that I could adapt to a test in the gem.

@hotoolong, I took a crack at that approach in #5523 and found that tests failed in some strange way (I don't really understand it -- I think those are the same failures that resulted in the code that's currently there.)

I also took another approach in #5524 which might also solve this problem in another way.

Please let me know if you can share any details about how I might check for this problem being solved. Also, feel free to try any of those other branches. I'd like to add a test to graphql-ruby before merging one of them, but you can let me know if either of them work for you.

@chaadow
Copy link
Author

chaadow commented Feb 4, 2026

@rmosolgo Thanks for taking the time to review my PR.

Here is the script i'm using to detect this

I've tried both PRs, they still autoloaded constants for me at bootup

# frozen_string_literal: true

# Extracted and adapted from this talk from Ben Sheldon:
# `An ok compromise. Faster development by designing for the Rails Autoloader`
# Youtube video link: https://youtu.be/9-PWz9nbrT8?si=Lw7qsF2_VmBperId&t=1487

require_relative "../../config/application"

autoloaded_constants = []

Rails.autoloaders.each do |loader|
  loader.on_load do |cpath, _value, _abspath|
    autoloaded_constants << [cpath, caller]
  end
end

Rails.application.initialize!

autoloaded_constants.each do |x|
  x[1] = Rails.backtrace_cleaner.clean(x[1]).first
end

allow_listed_constants = %w[
  HeicPreviewer
  ActionText::ContentHelper
  Statesman::Adapters::CustomActiveRecord
]

puts
if autoloaded_constants.reject! { _1.first.in?(allow_listed_constants) }.any?
  puts
  puts "ERROR: Autoloaded constants were referenced during during boot."
  puts
  puts "These files/constants were autoloaded during the boot process, which will result in" \
       "inconsistent behavior and will slow down and may break development mode. " \
       "Remove references to these constants from code loaded at boot. "
  puts
  puts
  w = autoloaded_constants.map { _1.first.length }.max
  autoloaded_constants.each do |name, location|
    puts "`#{name.ljust(w)}` referenced by #{location}"
  end

  exit 1
else
  puts "SUCCESS! No autoloaded constants were found during the boot process."
  exit 0
end

@hotoolong
Copy link

Thanks for trying the initializer approach in #5523!

I was wrong about how Rails::Engine route registration works - it happens when the Engine class is defined, not when the initializer runs. So initializer(..., before: :add_routing_paths) doesn't help here.

Looking at #5524, I think that approach is more robust. By separating the controllers into their own files with autoload, it defers the ActionController::Base subclass definitions until the Dashboard is actually used.

The concern I have with on_load(:action_controller) is that if another gem loads ActionController early (before Zeitwerk is set up), the same issue could occur. #5524 avoids this because the controller classes won't be defined until they're explicitly accessed, regardless of when ActionController itself is loaded.

@rmosolgo
Copy link
Owner

rmosolgo commented Feb 4, 2026

Thanks for sharing that script, @chaadow. I adapted it to the dummy application in this gem's tests in 86ad4b9.

However, in that scenario, the script succeeds -- no autoloaded constants are loaded during .initialize!. So, I assume the dummy app is missing some setup from your real production app. Could you help me track down what's missing? I'd like to get this fixed, but I think the first step will be replicating the failure somehow.

One possibility would be to dump the whole backtrace instead of the just first line (which points to the script itself). What if you delete this code:

- autoloaded_constants.each do |x|
-   x[1] = Rails.backtrace_cleaner.clean(x[1]).first
- end

And re-run it locally. What backtraces do you get for those autoloaded constants? I bet they're very similar. If you can share a couple of them, it might help us track down what exactly is causing them to be loaded in your app (but not in mine).

@rmosolgo
Copy link
Owner

rmosolgo commented Feb 6, 2026

I just updated #5524 to also use autoload for several other controllers, and I merged it to master. Could you try your script again now?

If it's still not working properly for you, please share some more debugging info (perhaps along the lines of my suggestion above) and we can keep hunting this issue! I'm going to close this PR for now but happy to keep discussing here.

@rmosolgo rmosolgo closed this Feb 6, 2026
@chaadow
Copy link
Author

chaadow commented Feb 6, 2026

@rmosolgo thank you very much, i've tested the master branch with your PR and it works, there is no longer any turbo-rails constants, since ActionController::Base is no longer called at require time!

@rmosolgo
Copy link
Owner

rmosolgo commented Feb 6, 2026

Glad to hear it! Thanks for letting me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants