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

Add eager loading #980

Merged
merged 1 commit into from
Apr 9, 2015
Merged

Add eager loading #980

merged 1 commit into from
Apr 9, 2015

Conversation

u2
Copy link
Contributor

@u2 u2 commented Apr 7, 2015

@dblock
Copy link
Member

dblock commented Apr 7, 2015

I definitely think we want to have this in CHANGELOG, please. Would you also care to provide some explanation of why you think this could be better?

@u2
Copy link
Contributor Author

u2 commented Apr 7, 2015

To be clear, the question you should be asking is: "if I don't eager load this module, when will it be autoloaded"? If the answer is: "during a request", you have to eager load it.

@dblock
Copy link
Member

dblock commented Apr 7, 2015

I am sold ;)

@u2
Copy link
Contributor Author

u2 commented Apr 7, 2015

Sorry, what's the meaning of I am sold? I don't understand some English slang.
For example:

config.autoload_paths += %W(#{config.root}/extras)

# root/extras/foo.rb
class Foo
end

# root/app/models/blog.rb
class Blog < ActiveRecord::Base
end

Things are ok in development

defined?(Blog)
# => nil 
defined?(Foo)
# => nil 

Blog
# => Blog (call 'Blog.connection' to establish a connection) 
Foo
# => Foo 

defined?(Blog)
# => "constant" 
defined?(Foo)
# => "constant"

But on the production, the situation is a little different…

defined?(Blog)
# => "constant"

defined?(Foo)
# => nil

Blog
# => Blog (call 'Blog.connection' to establish a connection) 
Foo
# => Foo 

defined?(Blog)
# => "constant" 
defined?(Foo)
# => "constant"

Even before we try to use Blog for the very first time, the class is already loaded and the constant is known. Why is that so? Because of eager loading.

@u2
Copy link
Contributor Author

u2 commented Apr 7, 2015

:), I will add CHANGELOG later.

@dblock
Copy link
Member

dblock commented Apr 7, 2015

"I am sold" means "you sold me the reason to do this", which means "I agree with this change because your argument makes sense". So yes, update CHANGELOG and we're good to go.

@u2
Copy link
Contributor Author

u2 commented Apr 8, 2015

updated.

@@ -14,7 +15,6 @@
* [#957](https://github.com/intridea/grape/pull/957): Regexp validator now supports `allow_blank`, `nil` value behavior changed - [@calfzhou](https://giihub.com/calfzhou).
* [#962](https://github.com/intridea/grape/pull/962): The `default` attribute with `false` value is documented now - [@ajvondrak](https://github.com/ajvondrak).
* [#974](https://github.com/intridea/grape/pull/974): Add error! to rescue_from blocks - [@whatasunnyday](https://github.com/whatasunnyday).
* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this back please?

@u2
Copy link
Contributor Author

u2 commented Apr 9, 2015

ok.

dblock added a commit that referenced this pull request Apr 9, 2015
@dblock dblock merged commit 5974d80 into ruby-grape:master Apr 9, 2015
@dblock
Copy link
Member

dblock commented Apr 9, 2015

Merged.

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.

2 participants