don't eagerload paths that have been flagged as eager_load => false #8146

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@josh-m-sharpe
Contributor

josh-m-sharpe commented Nov 8, 2012

Hey @spastorino,

This has created a regression in how paths are eager loaded:

53778ec

However, I don't think there's anything wrong with this commit.... I'll explain more:

Before the above commit:

?> pp Rails.configuration.paths.eager_load
["/home/jsharpe/project/app/assets",
"/home/jsharpe/project/app/controllers",
"/home/jsharpe/project/app/helpers",
"/home/jsharpe/project/app/models",
"/home/jsharpe/project/app/modules",
"/home/jsharpe/project/app/structs",
"/home/jsharpe/project/app/workers"]

After the commit:

?> pp Rails.configuration.paths.eager_load
["/home/jsharpe/project/app/controllers",
"/home/jsharpe/project/app/helpers",
"/home/jsharpe/project/app/models",
"/home/jsharpe/project/app/modules",
"/home/jsharpe/project/app/structs",
"/home/jsharpe/project/app/workers",
"/home/jsharpe/project/app"]

Note that /assets is now missing from #eager_load - which was the intent of the patch. However, /app is now included when it wasn't before.

I'm pretty sure that it was a bug before the commit that /app was missing. It is set to be eager_load => true here:

https://github.com/rails/rails/blob/3-2-rel/railties/lib/rails/engine/configuration.rb#L42

Now that /app is (correctly) part of eager_load, it now automatically loads anything inside of /app - including things that are specifically intended not be loaded.

For example, if I do this in config/ application.rb:

config.paths.add('app/manifests', :eager_load => false)

then files located in app/manifests are loaded - and that I believe is a regression. The attached patch fixes that.

I'm trying to set up the rails test suite up and running, but I wanted to at least start this conversation.

Thanks!

@@ -436,7 +436,7 @@ def eager_load!
config.eager_load_paths.each do |load_path|
matcher = /\A#{Regexp.escape(load_path)}\/(.*)\.rb\Z/
Dir.glob("#{load_path}/**/*.rb").sort.each do |file|
- require_dependency file.sub(matcher, '\1')
+ require_dependency file.sub(matcher, '\1') unless config.paths.skip_eager_load.any?{|path| file.match(path)}

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 8, 2012

Member

If we need to exclude something we need to fix filter_by, this is not the proper place

@rafaelfranca

rafaelfranca Nov 8, 2012

Member

If we need to exclude something we need to fix filter_by, this is not the proper place

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 8, 2012

Member

Yes. I agree that this is wrong. We need to fix it first on master and backport to 3-2-stable.

Member

rafaelfranca commented Nov 8, 2012

Yes. I agree that this is wrong. We need to fix it first on master and backport to 3-2-stable.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 8, 2012

Contributor

Thanks. However, I don't think the solution is proper. The question we need to answer is, why is app appearing on eager_load paths and make it no longer appear in there.

Contributor

josevalim commented Nov 8, 2012

Thanks. However, I don't think the solution is proper. The question we need to answer is, why is app appearing on eager_load paths and make it no longer appear in there.

@spastorino spastorino closed this in fc5ccd2 Nov 9, 2012

spastorino added a commit that referenced this pull request Nov 9, 2012

@spastorino spastorino reopened this Nov 9, 2012

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 9, 2012

Member

@spastorino thank you

Member

rafaelfranca commented Nov 9, 2012

@spastorino thank you

@spastorino

This comment has been minimized.

Show comment Hide comment
@spastorino

spastorino Nov 9, 2012

Member

This needs to be fixed in master, unsure if I would change this in 3-2-stable

Member

spastorino commented Nov 9, 2012

This needs to be fixed in master, unsure if I would change this in 3-2-stable

@fgrehm

This comment has been minimized.

Show comment Hide comment
@fgrehm

fgrehm Nov 11, 2012

Hey all,

Just for the record, ran into an issue when trying to setup activeadmin on a new app using 3.2.9.rc2 that might be related to this as it seems to be fixed with rc3.

I was going to create an issue over there and ended up creating this repo to showcase the bug. Now that I found out that the problem was probably not activeadmin I thought that you guys might be interested ;-)

Steps to reproduce:

$ git clone git://github.com/fgrehm/active_admin-issue.git
$ cd active_admin-issue
$ git checkout 781793c
$ bundle
$ rake db:migrate
  # Expected active_admin-issue/app/admin/admin_user.rb to define Admin::AdminUser 

The rails-3.2.9.rc3 branch have the changes I made on the master branch to make the gem work with rc2 reverted and works like a charm.

Cheers!

/cc @gregbell

fgrehm commented Nov 11, 2012

Hey all,

Just for the record, ran into an issue when trying to setup activeadmin on a new app using 3.2.9.rc2 that might be related to this as it seems to be fixed with rc3.

I was going to create an issue over there and ended up creating this repo to showcase the bug. Now that I found out that the problem was probably not activeadmin I thought that you guys might be interested ;-)

Steps to reproduce:

$ git clone git://github.com/fgrehm/active_admin-issue.git
$ cd active_admin-issue
$ git checkout 781793c
$ bundle
$ rake db:migrate
  # Expected active_admin-issue/app/admin/admin_user.rb to define Admin::AdminUser 

The rails-3.2.9.rc3 branch have the changes I made on the master branch to make the gem work with rc2 reverted and works like a charm.

Cheers!

/cc @gregbell

rafaelfranca added a commit that referenced this pull request Dec 18, 2012

Revert "Merge pull request #7587 from elia/fix-too-eager-loading"
This reverts commit 3663057.

REASON: This caused a regression that add app folder in the eager load
path. See #8146 for more information.

Conflicts:
	railties/CHANGELOG.md
@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 18, 2012

Member

Related with #7587

Member

rafaelfranca commented Dec 18, 2012

Related with #7587

@ghost ghost assigned rafaelfranca Jan 21, 2013

@arthurnn

This comment has been minimized.

Show comment Hide comment
@arthurnn

arthurnn Mar 1, 2014

Member

@rafaelfranca Should we close this, and if the behaviour is still incorrect open a new PR against master?

Member

arthurnn commented Mar 1, 2014

@rafaelfranca Should we close this, and if the behaviour is still incorrect open a new PR against master?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 2, 2015

Member

@arthurnn right.

Member

rafaelfranca commented Jan 2, 2015

@arthurnn right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment