require parent Tracing module before defining children

[zvkemp]

(Continuing discussion from #5178)

We've had a report that this change breaks constant loading if any of the nested files are required during app boot (I can open a new issue on this if necessary).

Here's a minimal reproduction:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'graphql', '2.4.7'
end

require 'graphql'
require 'graphql/tracing/active_support_notifications_trace'

GraphQL.eager_load!

What I think is happening is that the nested graphql/tracing/**.rb files don't require their parent (tracing.rb). So requiring this file:

• calls require "graphql/tracing/platform_trace",
• which loads platform_trace.rb,
• which encounters the autoload-registered constant GraphQL::Tracing
• which calls `require 'graphql/tracing'
• which calls require 'graphql/tracing/platform_trace', which is skipped because this file has already been required — at this point the loading of the PlatformTrace module is higher up in the callstack, so it's not going to complete until after the rest of tracing.rb is loaded
• Unfortunately, the rest of tracing.rb includes files that refer to PlatformTrace, so this error is raised:
  NameError: uninitialized constant GraphQL::Tracing::AppOpticsTrace::PlatformTrace

I believe all of these nested files should include a top-level require 'graphql/tracing' to ensure the order is correct.

[zvkemp]

^ Obviously tests are failing there — will address shortly. It's also throwing warnings about the circular requires, though I'm not sure there's a way to avoid that given the current structure.

[rmosolgo]

Hey, thanks for the detailed report and this suggested fix. The same kind of problem popped up here: #5188

I'm open to supporting these single-file requires, but do you mind sharing why it's important? For example, what if you remove that require "graphql/tracing/active_support_notifications_trace" from your app?

I'm open to supported these nested requires, but I'd like to understand a little more about why they're necessary. (I'd expect require "graphql" to make GraphQL::Tracing::ActiveSupportNotificationsTrace available.)

[zvkemp]

Well we have a library that requires "graphql/tracing/active_support_notifications_trace", so while we can remove it, there are already published versions that refer to that file specifically, it's the only core tracing functionality we need (so it makes sense to have a targeted require), and we didn't anticipate this moving to an autoload-based strategy.

I'm not really sure of the utility of using autoloading here in the first place — as the readme correctly points out, this isn't recommended in production, so is this just a development/test mode optimization? FWIW I would happily trade the 160ms saved for more predictable loading behavior.

[rmosolgo]

Yeah, that's fair -- if it's code that's already out there, then let's make it work again.

To clarify, do you need all these traces, or just ActiveSupportNotificationsTrace?

[rmosolgo]

To avoid the circular reference, I updated them all to require "graphql/tracing/platform_trace". It looks to me like this'll work -- what do you think, @zvkemp ?

[zvkemp]

I think the underlying issue is probably still the same — the file I used in my example already requires platform_trace.

[rmosolgo]

Oh, I see -- my test wasn't enough because it didn't require "graphql" first 😖 . I'll take another look.

[rmosolgo]

I slathered on some more autoload and I think it's working now 😅