Skip to content
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

Use prepend to patch Action Cable classes #310

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

palkan
Copy link
Contributor

@palkan palkan commented May 6, 2020

To preserve the original functionality a better monkey-patching technique than copying the code should be used, e.g.,Module#prepend.

NOTE: The original Action Cable code for Connection class has been already changed, thus Lograge breaks the core functionality:

  rescue ActionCable::Connection::Authorization::UnauthorizedError
-  respond_to_invalid_request		
+  close(reason: ActionCable::INTERNAL[:disconnect_reasons][:unauthorized], reconnect: false) if websocket.alive?
  end

Fixes #304.
Related to #294.

@palkan palkan force-pushed the fix/action-cable-patching branch 2 times, most recently from 143767a to 41a772a Compare May 6, 2020 15:46
@danielnc
Copy link

bump! we are seeing this issue too

@cin210
Copy link

cin210 commented Sep 3, 2020

We're able to reproduce this as well. Good fix!

@palkan
Copy link
Contributor Author

palkan commented Sep 3, 2020

Yes, this issue still makes Lograge incompatible with AnyCable; we're currently using this patch.

@jefflub
Copy link

jefflub commented Sep 8, 2020

Bump! Now using a patched version of lograge to work around this. Thanks @palkan!

@iloveitaly
Copy link
Collaborator

@palkan could you rebase this on master so CI can run? Can you add an entry to the changelog for this too?

Would love to get this merged in.

@palkan palkan force-pushed the fix/action-cable-patching branch from 41a772a to 103d827 Compare January 11, 2022 16:59
@daande
Copy link

daande commented Jan 11, 2022

We are seeing builds failing now since we pinned to commit 41a772a31ce39c6989d6d6e8c736e72247f2b620 I guess we can use the branch name instead. Would like to see this merged in asap please 💯

@palkan
Copy link
Contributor Author

palkan commented Jan 11, 2022

We are seeing builds failing now

Oops, sorry 🙄 (Highly recommend to fork all source-based dependencies to avoid such problems in the future)

To preserve the original functionality a better monkey-patching technique than copying the code should be used, e.g., Module#prepend

Fixes roidrage#304
@palkan palkan force-pushed the fix/action-cable-patching branch from 103d827 to 2c1a849 Compare January 20, 2022 14:58
@iloveitaly
Copy link
Collaborator

@palkan Awesome change! Mind submitting another PR with a changelog entry?

Hoping to cut a new release soon.

@iloveitaly iloveitaly merged commit 2fd03f7 into roidrage:master Jan 24, 2022
@palkan palkan deleted the fix/action-cable-patching branch January 25, 2022 07:33
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.

Issue with AnyCable: undefined method `protocol' for nil:NilClass
6 participants