Proposal: Add onReconnected to ActionCableLink

[d4rky-pl]

Warning

I am not sure if this is the best possible solution but this is what I came up with so far and it seems to be solving an actual, user-reported bug in a production application (applied in project with patch-package).

ActionCable and websockets in general are woefully unstable and prone to disconnecting the moment user navigates away from the tab or backgrounds a native app. This poses a serious problem in certain cases as it may cause the UI to become stale.

This is a very clearly userland problem and needs to be solved on the application level as each application may have different requirements on the freshness of the data. With that said, it would be great to have a generic way of detecting that the websocket has reconnected that is consistent with how the rest of the stack works.

Since Rails 7.1, ActionCable has an undocumented (poorly documented?) feature which allows us to do that.

Proposal

Add onReconnected option to ActionCableLink and call it in connected ActionCable callback if it received the reconnected argument. This will allow the user of the library to implement their own solution for data loss due to losing connection. It will only work in Rails 7.1 and just do nothing in previous versions.

The most basic way of using this feature later is to pair it with reFetchObservableQueries (but there are other, more targeted options if needed):

// other code unrelated to the problem
const link = ApolloLink.split(
  hasSubscriptionOperation,
  new ActionCableLink({
    cable,
    onReconnected: () => client.reFetchObservableQueries()
  }),
  httpLink
)

const client = new ApolloClient({ link, cache: new InMemoryCache() })

[rmosolgo]

Hey, thanks for the suggestion. I'm definitely interested in supporting this better.

Instead of adding a new, custom option, what if we provided direct access to the callbacks object here:

graphql-ruby/javascript_client/src/subscriptions/ActionCableLink.ts

Lines 35 to 57 in b664062

	}, connectionParams), {
	connected: function() {
	this.perform(
	actionName,
	{
	query: operation.query ? print(operation.query) : null,
	variables: operation.variables,
	// This is added for persisted operation support:
	operationId: (operation as {operationId?: string}).operationId,
	operationName: operation.operationName
	}
	)
	},
	received: function(payload) {
	if (payload?.result?.data || payload?.result?.errors) {
	observer.next(payload.result)
	}
	if (!payload.more) {
	observer.complete()
	}
	}
	})

Maybe instead of hard-coding that with just connected and received, we could accept an object from the caller and insert our own handlers as needed. (If the user provides a function for either of those callbacks, we extract it, then create a new function which calls both the user-provided one and the library-provided one.)

I think that would make it easier to understand exactly what's going on, and besides, maybe there'd be other handy things about having direct access to those callbacks. What do you think?

[d4rky-pl]

@rmosolgo I've been thinking about this problem since yesterday. I like this approach and I was considering it as well, just couldn't find a use-case for overriding received so I went with the YAGNI approach but what you're saying makes more sense from the library API point of view. The only thing I'm wondering is if we should also allow the original actions done by ActionCableLink to be prevented from triggering (either implicitly by something like return false or explicitly by passing a second argument to the callbacks with { preventDefault }).

I'm still wrapping my head around this and testing it in an actual application, and at this point I'm no longer sure if this is solving the "stale UI because of broken connection in the background" problem. I had a bug report today that it didn't, still investigating (shouldn't have disabled the debug logger so quickly...).

I'll get back to you after some days and after implementing subscriptions in a larger application, it should give me a better idea on whether this is the right place and the right feature.

[rmosolgo]

Sounds good -- we can pursue a change like that in a new PR if you find that it would address your issue 👍