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

ActionCable Postgres adapter is ActiveRecord-specific #27214

Open
bgentry opened this Issue Nov 29, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@bgentry
Contributor

bgentry commented Nov 29, 2016

Steps to reproduce

  1. Create a Rails app using Sequel and sequel-rails, without ActiveRecord
  2. Attempt to use the ActionCable Postgres adapter
  3. Watch it crash trying to autoload ActiveRecord

Expected behavior

Ideally, this would support the most popular Postgres ORMs out of the box. Or at least the docs should mention that this only works with AR.

Actual behavior

Exception trying to autoload ActiveRecord

System configuration

Rails version: 5.0.0.1

Ruby version: 2.3.3

It was very easy to fork the Postgres ActionCable subscription adapter to work with Sequel. I only had to alter the with_connection method, which is even shorter with Sequel: https://gist.github.com/bgentry/5a4592dbbcc398c0ad651c53af7da51f#file-postgresql_sequel-rb-L35-L40

I'm not sure whether the right approach is to try to support this out-of-the-box, or at least to document that the adapter is AR-specific. Thoughts?

As a user I'd certainly prefer not to have to maintain a fork of the subscription adapter. But I can certainly see the argument for this not being Rails' problem. I suppose I really only need to monkey-patch or refine that one method, though, so not too bad either way.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 29, 2016

Member

That is a good point. I don't see why this code is using Active Record at all, it should not, even more that actioncable doesn't depend on activerecord and active record base is not being required in that file.

In my opinion we should use the pg connection directly.

Member

rafaelfranca commented Nov 29, 2016

That is a good point. I don't see why this code is using Active Record at all, it should not, even more that actioncable doesn't depend on activerecord and active record base is not being required in that file.

In my opinion we should use the pg connection directly.

@bgentry

This comment has been minimized.

Show comment
Hide comment
@bgentry

bgentry Nov 29, 2016

Contributor

@rafaelfranca my guess is it's because it's piggybacking on the AR database config logic. An alternative approach to supporting any ORM is to have a config setting that lets me specify how to get a new PG connection, defaulting to the AR method used currently.

I did notice the lack of a require at the top which is maybe a separate but related issue?

Contributor

bgentry commented Nov 29, 2016

@rafaelfranca my guess is it's because it's piggybacking on the AR database config logic. An alternative approach to supporting any ORM is to have a config setting that lets me specify how to get a new PG connection, defaulting to the AR method used currently.

I did notice the lack of a require at the top which is maybe a separate but related issue?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 29, 2016

Member

I prefer to use the connection directly over supporting different ORM. There is no need for a ORM in actioncable.

We can't add the require without changing the dependencies of actioncable to add activerecord, so I prefer that we solve this issue removing any orm specific code.

Member

rafaelfranca commented Nov 29, 2016

I prefer to use the connection directly over supporting different ORM. There is no need for a ORM in actioncable.

We can't add the require without changing the dependencies of actioncable to add activerecord, so I prefer that we solve this issue removing any orm specific code.

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Nov 29, 2016

Member

Thank you for opening up this issue! Working on a solution locally now.

Member

maclover7 commented Nov 29, 2016

Thank you for opening up this issue! Working on a solution locally now.

@maclover7 maclover7 self-assigned this Nov 29, 2016

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Nov 29, 2016

Member

Opened up #27216 as a possible solution for this.

Member

maclover7 commented Nov 29, 2016

Opened up #27216 as a possible solution for this.

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