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

Release 1.5.0 breaks rails app boot with eager-loading #183

Closed
timon opened this issue Oct 17, 2022 · 8 comments · Fixed by #184
Closed

Release 1.5.0 breaks rails app boot with eager-loading #183

timon opened this issue Oct 17, 2022 · 8 comments · Fixed by #184

Comments

@timon
Copy link

timon commented Oct 17, 2022

Describe the bug

We have noticed the following failure in dependabot PR that bumps version of dry-struct to 1.5.0:

Run bundle exec rspec spec

An error occurred while loading ./spec/controllers/api/v1/creditsafe/search_spec.rb.
Failure/Error: require File.expand_path('../config/environment', __dir__)
NameError:
  uninitialized constant Dry::Struct::Extensions

# ./vendor/bundle/ruby/3.0.0/gems/dry-core-0.9.0/lib/dry/core/deprecations.rb:227:in `block (2 levels) in deprecate_constant'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader/helpers.rb:127:in `const_get'
</summary>
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader/helpers.rb:127:in `cget'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader.rb:246:in `block (2 levels) in eager_load'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader/helpers.rb:41:in `block in ls'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader/helpers.rb:27:in `each'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader/helpers.rb:27:in `ls'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader.rb:234:in `block in eager_load'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader.rb:219:in `synchronize'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader.rb:219:in `eager_load'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader.rb:318:in `each'
# ./vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.6.1/lib/zeitwerk/loader.rb:318:in `eager_load_all'
# ./vendor/bundle/ruby/3.0.0/gems/railties-7.0.4/lib/rails/application/finisher.rb:74:in `block in <module:Finisher>'
# ./vendor/bundle/ruby/3.0.0/gems/railties-7.0.4/lib/rails/initializable.rb:32:in `instance_exec'
# ./vendor/bundle/ruby/3.0.0/gems/railties-7.0.4/lib/rails/initializable.rb:32:in `run'
# ./vendor/bundle/ruby/3.0.0/gems/railties-7.0.4/lib/rails/initializable.rb:61:in `block in run_initializers'
# ./vendor/bundle/ruby/3.0.0/gems/railties-7.0.4/lib/rails/initializable.rb:60:in `run_initializers'
# ./vendor/bundle/ruby/3.0.0/gems/railties-7.0.4/lib/rails/application.rb:372:in `initialize!'
# ./config/environment.rb:7:in `<top (required)>'
# ./spec/rails_helper.rb:11:in `require'
# ./spec/rails_helper.rb:11:in `<top (required)>'
# ./spec/controllers/api/v1/creditsafe/search_spec.rb:3:in `require'
# ./spec/controllers/api/v1/creditsafe/search_spec.rb:3:in `<top (required)>'

An error occurred while loading ./spec/controllers/api/v1/documentation_controller_spec.rb.
Failure/Error: require File.expand_path('../config/environment', __dir__)

To Reproduce

A minimalistic rails app that exhibits this behavior is available at timon/dry-failures#1

Please note that

  • CI should be present at env to enforce eager loading of rails app in a test environment
  • Just adding dry-core seems to work fine

Expected behavior

Updating to dry-struct should not break eager loading of rails apps, neither in CI nor in production mode

My environment

  • Affects my production application: YES
  • Ruby version: 3.0.4
  • OS: MacOS, ubuntu-latest in github runner
@texpert
Copy link

texpert commented Oct 17, 2022

I am getting similar uninitialized constant Dry::Struct::Extensions (NameError) errors.

@flash-gordon
Copy link
Member

Gemfile doesn't have anything dry-related, what am I missing?

@timon
Copy link
Author

timon commented Oct 17, 2022

@flash-gordon I added a link to a PR that demonstrates how adding dry-struct to gemfile breaks the CI / app eager loading, please check the commits referenced in timon/dry-failures#1

@solnic
Copy link
Member

solnic commented Oct 17, 2022

I figured it out. This would fix it:

diff --git lib/dry/struct.rb lib/dry/struct.rb
index a6d4de6..aac1261 100644
--- lib/dry/struct.rb
+++ lib/dry/struct.rb
@@ -99,12 +99,16 @@ module Dry
           loader.push_dir(root)
           loader.ignore(
             "#{root}/dry-struct.rb",
-            "#{root}/dry/struct/{class_interface,errors,extensions,printer,value,version}.rb"
+            "#{root}/dry/struct/{class_interface,errors,printer,value,version}.rb"
           )
         end
       end
     end
 
+    register_extension(:pretty_print) do
+      require "dry/struct/extensions/pretty_print"
+    end
+
     loader.setup
 
     include ::Dry::Equalizer(:__attributes__, inspect: false, immutable: true)
@@ -220,6 +224,5 @@ module Dry
   end
 end
 
-require "dry/struct/extensions"
 require "dry/struct/printer"
 require "dry/struct/value"
diff --git lib/dry/struct/extensions.rb lib/dry/struct/extensions.rb
deleted file mode 100644
index 7584e67..0000000
--- lib/dry/struct/extensions.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-# frozen_string_literal: true
-
-Dry::Struct.register_extension(:pretty_print) do
-  require "dry/struct/extensions/pretty_print"
-end
diff --git lib/dry/struct/extensions/pretty_print.rb lib/dry/struct/extensions/pretty_print.rb
index 262a5a9..49b21df 100644
--- lib/dry/struct/extensions/pretty_print.rb
+++ lib/dry/struct/extensions/pretty_print.rb
@@ -4,19 +4,25 @@ require "pp"
 
 module Dry
   class Struct
-    def pretty_print(pp)
-      klass = self.class
-      pp.group(1, "#<#{klass.name || klass.inspect}", ">") do
-        pp.seplist(@attributes.keys, proc { pp.text "," }) do |column_name|
-          column_value = @attributes[column_name]
-          pp.breakable " "
-          pp.group(1) do
-            pp.text column_name.to_s
-            pp.text "="
-            pp.pp column_value
+    module Extensions
+      module PrettyPrint
+        def pretty_print(pp)
+          klass = self.class
+          pp.group(1, "#<#{klass.name || klass.inspect}", ">") do
+            pp.seplist(@attributes.keys, proc { pp.text "," }) do |column_name|
+              column_value = @attributes[column_name]
+              pp.breakable " "
+              pp.group(1) do
+                pp.text column_name.to_s
+                pp.text "="
+                pp.pp column_value
+              end
+            end
           end
         end
       end
     end
+
+    include(Extensions::PrettyPrint)
   end
 end

😬 @flash-gordon thoughts?

@solnic
Copy link
Member

solnic commented Oct 17, 2022

Actually, here's a PR #184

Notice that it leads to dry-core but it's just because its deprecations module defines const_missing and it's just misled us thinking that it's a problem with dry-core, when in reality it was because we added extensions to ignore list in Zeitwerk config, and that actually messed up auto-loading of Dry::Core::Extensions API that Dry::Struct is extended by.

@timon
Copy link
Author

timon commented Oct 17, 2022

@solnic Thanks, I've added a commit to a sample up to use your branch, and everything works there.
I tried out refering the same version in our actuall app, and it works there as well 🎉

solnic added a commit that referenced this issue Oct 17, 2022
solnic added a commit that referenced this issue Oct 17, 2022
…files

[changelog]

version: 1.5.1
date: 2022-10-17
fixed: "Fixed issues with auto-loading `Extensions` module (issue #183 fixed via #184) (@solnic)"
@solnic
Copy link
Member

solnic commented Oct 17, 2022

@timon this is now fixed in 1.5.1. Sorry for the trouble and thanks for the helpful reproduction repo 🙂

@flash-gordon
Copy link
Member

@timon oops I missed it, just clicker at the repo. Thanks for the repro, it always helps

cllns pushed a commit to cllns/dry-struct that referenced this issue Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants