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

Inject Rails related configuration through Railtie #23505

Merged
merged 3 commits into from Feb 14, 2016

Conversation

Projects
None yet
4 participants
@kaspth
Member

kaspth commented Feb 5, 2016

Instead of direct coupling we can inject configuration. This also spares us some setup in tests.

Work in progress. Need to consider better error messages, better docs, tests.

Would also like to tackle the inner_logger + TaggedLoggerProxy.new test setup, but unsure what to do there at the moment. Maybe we can move new_tagged_logger to the server?

cc @rafaelfranca @matthewd

@kaspth kaspth added the actioncable label Feb 5, 2016

@kaspth kaspth self-assigned this Feb 5, 2016

@kaspth kaspth added this to the 5.0.0 milestone Feb 5, 2016

@matthewd

View changes

actioncable/lib/action_cable/engine.rb Outdated
@@ -31,6 +31,12 @@ class Railtie < Rails::Engine # :nodoc:
self.cable = Rails.application.config_for(config_path).with_indifferent_access
end
channel_load_paths << Rails.root.join('app/channels')
if defined?(ApplicationCable::Connection)

This comment has been minimized.

@matthewd

matthewd Feb 6, 2016

Member

I'm pretty sure this won't work, because of autoloading.. right?

This comment has been minimized.

@kaspth

kaspth Feb 6, 2016

Member

Ah, right! Replacing with a string and safe_constantize.

@kaspth kaspth force-pushed the kaspth:inject-rails-config-through-railtie branch Feb 6, 2016

@kaspth

View changes

actioncable/lib/action_cable/engine.rb Outdated
else
raise RuntimeError, <<-eow.strip_heredoc
Unable to find an `ApplicationCable::Connection` for Action Cable to use.
Check the README for instructions on where and how to set it up.

This comment has been minimized.

@kaspth

kaspth Feb 6, 2016

Member

Quirky error message. Should be removed when we change rails:update to generate the connection.rb file.

server.config = ActionCable::Server::Configuration.new
inner_logger = Logger.new(StringIO.new).tap { |l| l.level = Logger::UNKNOWN }
server.config.logger = ActionCable::Connection::TaggedLoggerProxy.new(inner_logger, tags: [])
server.config.logger = Logger.new(StringIO.new).tap { |l| l.level = Logger::UNKNOWN }

This comment has been minimized.

@kaspth

kaspth Feb 6, 2016

Member

Couldn't remove this without seeing circular require warnings thrown in the Active Record connection management concern. Haven't yet figured out why that is.

@kaspth kaspth force-pushed the kaspth:inject-rails-config-through-railtie branch Feb 10, 2016

@kaspth

View changes

actioncable/lib/action_cable/engine.rb Outdated
@@ -31,6 +32,10 @@ class Railtie < Rails::Engine # :nodoc:
self.cable = Rails.application.config_for(config_path).with_indifferent_access
end
self.connection_class = ApplicationCable::Connection

This comment has been minimized.

@kaspth

kaspth Feb 10, 2016

Member

Killed the error message since from the point of the final release the file with this constant has been generated through rails:update.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2016

Member

Is it defined in config? We don't generate app files in that task

This comment has been minimized.

@kaspth

kaspth Feb 10, 2016

Member

It's supposed to be in app/channels/application_cable/connection.rb.

@rafaelfranca

View changes

actioncable/lib/action_cable/engine.rb Outdated
@@ -31,6 +32,10 @@ class Railtie < Rails::Engine # :nodoc:
self.cable = Rails.application.config_for(config_path).with_indifferent_access
end
self.connection_class = ApplicationCable::Connection
channel_load_paths << Rails.root.join('app/channels')

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2016

Member

We should get it from the Paths object.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2016

Member

The main reason is that doing this engines will just work.

This comment has been minimized.

@kaspth

kaspth Feb 10, 2016

Member

Ah, cool! You mean the root to join onto? I.e. Rails.application.paths.path.join('app/channels')?.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2016

Member

channel_load_paths = paths['app/channels'].existent

@rafaelfranca

View changes

actioncable/lib/action_cable/engine.rb Outdated
@@ -2,6 +2,7 @@
require "action_cable"
require "action_cable/helpers/action_cable_helper"
require "active_support/core_ext/hash/indifferent_access"
require "active_support/core_ext/string/strip"

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 10, 2016

Member

If you don't need the message this can be removed

This comment has been minimized.

@kaspth

kaspth Feb 10, 2016

Member

Doh! Good catch.

@kaspth kaspth force-pushed the kaspth:inject-rails-config-through-railtie branch Feb 10, 2016

@spastorino

This comment has been minimized.

Member

spastorino commented Feb 10, 2016

@kaspth looks great, 👍

@kaspth kaspth force-pushed the kaspth:inject-rails-config-through-railtie branch 2 times, most recently Feb 11, 2016

kaspth added some commits Feb 5, 2016

Inject Rails' channel paths in engine.
We were explicitly referencing Rails.root in ActionCable::Server::Configuration.initialize,
thereby coupling ourselves to Rails.

Instead add `app/channels` to Rails' app paths and assign the existent files
to `channel_paths`.

Users can still append to those load paths with `<<` and `push` in `config/application.rb`.

This means we can remove the custom `Dir` lookup in `channel_paths` and the Rails
and root definitions in the tests.
Default connection class to ActionCable::Connection::Base.
Instead of depending on ApplicationCable::Connection being defined at initialize
we should inject it in the Railtie.

Thus we can kill more setup in the tests too.
Don't rely on the global server as a receiver.
The `WorkerTest`'s `Receiver` is imporsonating an `ActionCable::Connection::Base`, but
just delegates the logger to `ActionCable.logger`.

This creates a mismatch as the connection requires the logger to be a
`TaggedLoggerProxy`'ied logger, while the server doesn't.

Thus to ensure an exception isn't raised when the worker tries to call `tag`
other tests have to assign a proxied logger to their test server.

Instead of forcing change on other tests, have Receiver adhere to the connection
contract and use a `TaggedLoggerProxy`.

As a consequence remove more setup from the tests.

@kaspth kaspth force-pushed the kaspth:inject-rails-config-through-railtie branch to 3ae8eb1 Feb 14, 2016

kaspth added a commit that referenced this pull request Feb 14, 2016

Merge pull request #23505 from kaspth/inject-rails-config-through-rai…
…ltie

Inject Rails related configuration through Railtie

@kaspth kaspth merged commit 3c96a6e into rails:master Feb 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaspth kaspth deleted the kaspth:inject-rails-config-through-railtie branch Feb 14, 2016

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Feb 15, 2016

Fuck love this! Thank you for working on this.

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 15, 2016

@rafaelfranca awww, ❤️❤️❤️

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