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

2.5.0 update causes new autoloading bug with #hash method. #188

Closed
ethn opened this issue Oct 20, 2021 · 5 comments
Closed

2.5.0 update causes new autoloading bug with #hash method. #188

ethn opened this issue Oct 20, 2021 · 5 comments

Comments

@ethn
Copy link

ethn commented Oct 20, 2021

I'm afraid I don't have time to generate a test, but the short version is that the call include Card::Env::Location triggers the autoloading of card/env.rb, which contains the method:

      def hash hashish
        case hashish
        when Hash then hashish.clone
        when ActionController::Parameters then hashish.to_unsafe_h
        else {}
        end
      end

full file here

After the 2.5.0 update, this generates the following new error:

ArgumentError: wrong number of arguments (given 0, expected 1)
backtrace:

/Users/ethan/dev/decko/gem/card/lib/card/env.rb:48:in `hash'
/Users/ethan/.gem/ruby/2.7.3/gems/zeitwerk-2.5.0/lib/zeitwerk/autoloads.rb:56:in `hash'
/Users/ethan/.gem/ruby/2.7.3/gems/zeitwerk-2.5.0/lib/zeitwerk/autoloads.rb:56:in `delete'
/Users/ethan/.gem/ruby/2.7.3/gems/zeitwerk-2.5.0/lib/zeitwerk/autoloads.rb:56:in `delete'
/Users/ethan/.gem/ruby/2.7.3/gems/zeitwerk-2.5.0/lib/zeitwerk/loader/callbacks.rb:11:in `on_file_autoloaded'
/Users/ethan/.gem/ruby/2.7.3/gems/zeitwerk-2.5.0/lib/zeitwerk/kernel.rb:28:in `block in require'
/Users/ethan/.gem/ruby/2.7.3/gems/zeitwerk-2.5.0/lib/zeitwerk/kernel.rb:27:in `tap'
/Users/ethan/.gem/ruby/2.7.3/gems/zeitwerk-2.5.0/lib/zeitwerk/kernel.rb:27:in `require'
/Users/ethan/dev/decko/gem/card/lib/card/format.rb:20:in `<class:Format>'
@fxn
Copy link
Owner

fxn commented Oct 20, 2021

Interesting, there's new code that assumes classes and modules that act as namespaces are hashable. But yours shows that assumption cannot be made.

Thanks for giving 2.5.0 a try so quickly, I'll think about it and will come back.

@ethn
Copy link
Author

ethn commented Oct 20, 2021

Awesome. Thank you!

Fwiw, there's not a heavy commitment to this code pattern in our case. Eg, if you were to come back and say that certain methods are incompatible with zeitwerk support, then we could work around it.

Looking forward to hearing what you decide.

@fxn
Copy link
Owner

fxn commented Oct 20, 2021

@ethn that's very helpful to know. I believe I know how to get rid of that hash, but if I am wrong, I'll take that into account ❤️.

@fxn fxn closed this as completed in c0bdbfb Oct 20, 2021
@fxn
Copy link
Owner

fxn commented Oct 20, 2021

I've published 2.5.1 with the fix.

Thanks again for testing 2.5.0 so quickly and reporting this regression :).

@ethn
Copy link
Author

ethn commented Oct 21, 2021

Wow, fantastic. Thanks for the quick fix!

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

No branches or pull requests

2 participants