Skip to content

Conversation

@kernow
Copy link
Contributor

@kernow kernow commented May 25, 2017

Rails convention is now to load application config from initializers rather than config/application.rb. This PR makes it possible to specifyserver_renderer_directories (amongst other settings) and have the settings honoured by the file watcher. Previously some settings configured in an initializer were ignored as the file watcher block was run before initializers were loaded.

@rmosolgo
Copy link
Member

👏 wow, thanks this looks great! let's see how CI likes it 😬

Nice to know I can have react-rails watch my pants directory 😆

@kernow
Copy link
Contributor Author

kernow commented May 25, 2017

Humm, lots of failures :( it's really hard to test this change as most of the methods for the active support file watchers are private and it's not possible to get any information about which files have been added to the watch list without accessing private data which is not consistent across all of the available file watchers.

The only other option I can think of to test this is to create a mock file watcher object for the test.

class RailtieTest < ActionDispatch::IntegrationTest
test "reloaders are configured after initializers are loaded" do
begin
@test_file = File.expand_path("../../dummy/app/pants", File.dirname(__FILE__))
Copy link
Member

Choose a reason for hiding this comment

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

This file name, does it end in .js or .jsx? I think only those files are watched due to config.react.server_renderer_extensions. Could that be why the test doesn't pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'd forgotten about the file extension. Although some of the tests are passing and they passed locally which is odd as you would expect it not to pass at all. I'll check it out

@kernow kernow force-pushed the load-watches-after-initializers branch from 958a2ed to a213ca7 Compare May 25, 2017 14:59
@kernow
Copy link
Contributor Author

kernow commented May 25, 2017

@rmosolgo this is much happier now. I assumed that all the builds would be passing on travis (before my changes) but it looks like there are several test that have not been passing on master. Is there anything else you want me to do on the PR?

@rmosolgo
Copy link
Member

No, I have to figure those ones out separately! I'll cut a release soon, I'm hoping to merge one more fix...

@rmosolgo rmosolgo merged commit b69c9ce into reactjs:master May 25, 2017
@rmosolgo
Copy link
Member

Finally shipped in 2.2.1!

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