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

ActionCableSubscription#perform now returns a promise #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmdoptesc
Copy link
Contributor

So this deviates from the ActionCable JS client a where it returns a boolean:
https://github.com/rails/rails/blob/242216803ee73f528b0a74c6bdb367e5b98d8f83/actioncable/app/assets/javascripts/action_cable.js#L318

Long trace of ActionCable.Subscriptions -> ActionCable.Consumer: https://github.com/rails/rails/blob/242216803ee73f528b0a74c6bdb367e5b98d8f83/actioncable/app/assets/javascripts/action_cable.js#L140

But for our purposes, we noticed sometimes AnyCable's ActionCableSubscription#perform is called on a closed subscription and raises an error (should now be adjustable in 0.9.2 thanks!), but it would be nice to catch and handle those errors.

Channel#perform already surfaces the promise, so why not just surface it one more level?

return this.receiver.perform(this.identifier, action, payload)

So this deviates from the ActionCable JS client a where it
returns a boolean:
https://github.com/rails/rails/blob/242216803ee73f528b0a74c6bdb367e5b98d8f83/actioncable/app/assets/javascripts/action_cable.js#L318

Long trace of ActionCable.Subscriptions -> ActionCable.Consumer:
https://github.com/rails/rails/blob/242216803ee73f528b0a74c6bdb367e5b98d8f83/actioncable/app/assets/javascripts/action_cable.js#L140

But for our purposes, we noticed sometimes AnyCable's
ActionCableSubscription#perform is called on a closed subscription
and raises an error (should now be adjustable in 0.9.2 thanks!),
but it would be nice to catch and handle those errors.

Channel#perform already surfaces the promise, so why not just
surface it one more level?
https://github.com/anycable/anycable-client/blob/62674a8207b6add893677369106766c7baa234f4/packages/core/channel/index.js#L120
@palkan
Copy link
Member

palkan commented Dec 13, 2024

So this deviates from the ActionCable JS client a where it returns a boolean:

Good catch. I think, a proper fix would be to return boolean as well (and check the connection state to return false when it's closed).

Otherwise our implementation would deviate even more from the Action Cable one.

That doesn't help with the original goal—catching errors. But for that, I think, the right way is to use AnyCable channels instead of the Action Cable wrapper.

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