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

Move convenience class into new file #2307

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

fixlr
Copy link
Contributor

@fixlr fixlr commented Feb 23, 2023

This PR moves the Grape::Types::InvalidValue class definition into a separate file so that it can be lazy loaded in projects that use zeitwerk file structure conventions.

In environments that do not eager load, referencing the Grape::Validations::Types::InvalidValue class before Grape::Types::InvalidValue causes a load error due to the fact that both classes were defined in the same file.


Testing?! Yes! Kind of...

I haven't come up with a good way to write a new rspec example for this change, but one test did fail on my branch before I updated lib/grape.rb. This failure occurred in coerce_spec.rb:271 because Grape::Types::InvalidValue was no longer being autoloaded. (It raised the same uninitialized constant Grape::Types exception that you'll see in my example below.)

For now, in lieu of tests, I created a simple gist that demonstrates the issue I'm seeing in a private Rails application. Here are the results on both my branch and master:

> git checkout master
> ruby grape_autoload_bug_sample.rb
grape_autoload_bug_sample.rb:15:in `<main>': uninitialized constant Grape::Types (NameError)

puts "Loaded #{Grape::Types::InvalidValue.name}"
                           ^^^^^^^^^^^^^^ 

> ruby grape_autoload_bug_sample_reversed.rb
Loaded Grape::Validations::Types::InvalidValue
Loaded Grape::Types::InvalidValue

> git checkout extract-grape-types-invalid_type
> ruby grape_autoload_bug_sample.rb
Loaded Grape::Types::InvalidValue
Loaded Grape::Validations::Types::InvalidValue

This commit moves the `Grape::Types::InvalidValue` class definition into
a separate file so that it can be lazy loaded in projects that use
zeitwerk file naming conventions.

In environments that do not eager load, referencing the
`Grape::Validations::Types::InvalidValue` class before
`Grape::Types::InvalidValue` causes a load error due to the fact that
both classes were defined in the same file.
@fixlr fixlr force-pushed the extract-grape-types-invalid_type branch from 067b7ec to fc89d00 Compare February 23, 2023 06:55
@fixlr fixlr marked this pull request as ready for review February 23, 2023 06:56
@grape-bot
Copy link

1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

@dblock
Copy link
Member

dblock commented Feb 23, 2023

Awesome, thank you. Let's merge this.

And thank you for trying to write a test for it. If you still feel motivated, I think you should be able to either extend https://github.com/ruby-grape/grape/tree/master/spec/integration/eager_load or make a new test that's similar. And if you feel even more extra motivated, we probably have multithreading bugs related to eager-loading, too.

@dblock dblock merged commit 4669556 into ruby-grape:master Feb 23, 2023
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.

None yet

3 participants