Skip to content

Reposition asset path build #347

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

Merged

Conversation

dv
Copy link
Contributor

@dv dv commented Aug 31, 2015

The standard config/initializers are only run after railtie#before_initialize, but we have to build the asset paths before railtie#after_initialize since at that point Sprockets has already frozen the asset paths.

This PR moves the asset path build to a point in between before_initialize and after_initialize, after the standard initializers are run but before Sprockets freezes the environment.

Fixes #345

The standard config/initializers are only run after `railtie#before_initialize`, but we have to build the asset paths before `railtie#after_initialize` since at that point Sprockets has already frozen the asset paths.

This PR moves the asset path build to a point in between `before_initialize` and `after_initialize`, after the standard initializers are run but before Sprockets freezes the environment.
@dv
Copy link
Contributor Author

dv commented Aug 31, 2015

I spent a few hours looking on how to best test the initializer code, but apparently this is almost impossible - since we would have to completely reboot the Rails app, and I couldn't find anywhere how to do that in a test.

The compromise I've used here is to hardcode an initializer in the dummy app, which overrides a setting set in the dummy app's application.rb. The single test that checks the path now has the added task of checking if the addons option was set before Sprockets froze the world.

@rmosolgo
Copy link
Member

rmosolgo commented Sep 3, 2015

Yeah, I remember trying to find some way to test it also, and basically giving up :P

But the way you tested it looks good to me. And it passes on all our Ruby & Rails versions :D

rmosolgo pushed a commit that referenced this pull request Sep 3, 2015
@rmosolgo rmosolgo merged commit 8696f50 into reactjs:master Sep 3, 2015
@rmosolgo
Copy link
Member

rmosolgo commented Sep 3, 2015

Thanks! 🎊

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.

3 participants