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

Fixes #3988 - use require instead of autoload_once_paths #1

Merged

Conversation

iNecas
Copy link

@iNecas iNecas commented Mar 14, 2014

The autoload_once_paths were used incorrectly. From http://guides.rubyonrails.org/configuring.html:

config.autoload_once_paths accepts an array of paths from which
Rails will autoload constants that won't be wiped per request.
Relevant if config.cache_classes is false, which is the case
in development mode by default. Otherwise, all autoloading happens
only once.
All elements of this array must also be in autoload_paths.

In our case, the paths are not in autoload_paths directly.

# Require the following paths only once in development environment
# this is useful to keep singletons from reloading.
if Rails.env.development?
Dir.glob(["#{Rails.root}/app/services/foreman/**/*.rb",

Choose a reason for hiding this comment

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

It might be better to load only the file(s) we need for the menu, if that's possible..

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to keep the previous behaviour as much as possible, to keep the risk of actually braking something, as low as possible :)

Choose a reason for hiding this comment

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

Go wild :)

Choose a reason for hiding this comment

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

I was a bit reluctant with this change in the first place, so the more we can have in the autoload path, and the least in the "never reload" path, the better it'll be for us devs.

Choose a reason for hiding this comment

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

Just discussed with @mbacovsky, I think the only two files we need to keep outside the autoloader are app/services/foreman/plugin.rb for plugin registrations and app/services/menu/manager.rb for added menu items.

Copy link
Author

Choose a reason for hiding this comment

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

After some testing, more files turned out to be required (all the menu stuff) to keep the singletons working with plugins. I've pushed updated version, keeping the minimum needed to be required.

The `autoload_once_paths` were used incorrectly. From http://guides.rubyonrails.org/configuring.html:

> config.autoload_once_paths accepts an array of paths from which
> Rails will autoload constants that won't be wiped per request.
> Relevant if config.cache_classes is false, which is the case
> in development mode by default. Otherwise, all autoloading happens
> only once.
> All elements of this array must also be in autoload_paths.

In our case, the paths are not in autoload_paths directly.
@domcleal
Copy link

👍 thanks @iNecas!

@mbacovsky
Copy link
Owner

@iNecas,
thank_you

mbacovsky added a commit that referenced this pull request Mar 17, 2014
Fixes theforeman#3988 - use require instead of autoload_once_paths
@mbacovsky mbacovsky merged commit babfb65 into mbacovsky:4515_bindings_support Mar 17, 2014
@iNecas
Copy link
Author

iNecas commented Mar 17, 2014

:D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants