The example for ActionCable subscription may contain a bug

[d4rky-pl]

Describe the bug

I figured this one out after a long session of debugging: if you skip the initial response from the GraphQL subscription then the example GraphqlChannel from the docs will fail to register the subscription and throw an error about missing field.

Versions

graphql version: 2.4.8
graphql-ruby-client version: 1.14.5

Steps to reproduce

Implement ActionCable integration exactly as in the documentation, then use useSubscribe from basic Apollo stack.

I can provide full repro if needed.

Expected behavior

No errors from Apollo library, subscription registers correctly

Actual behavior

Missing field 'subscriptionName' while writing result {}

The subscription fails to be attached properly.

Additional context

This error happens because the example implementation of GraphqlChannel by default converts data: nil into data: {} (due to .to_h) and then always calls transmit, pushing empty data in an unexpected format. This is in turned picked by ActionCableLink which checks if result.data exists but not if it's an empty object, passing it to Apollo, triggering the bug.

I am not sure if this is expected behaviour in any configuration (Relay?) so I did not send a pull request but the solution when using base Apollo is simply to:

transmit(payload) unless result['data'].empty?

This will avoid the initial, broken call while making all the subsequent subscription events work as expected.

[rmosolgo]

Hey, thanks for suggestion. Let's update the example according to your suggestion. It seems like the spec is vague on subscription initial responses, and my impression is that Apollo is more widely used than Relay, anyways. Do you mind opening a PR for it?

[d4rky-pl]

@rmosolgo I was going to send a PR but then I realized the same code is used in the system tests and modifying spec/dummy/app/channels/graphql_channel.rb:108 to include unless result["data"].empty? breaks them pretty badly with:

expected to find css "#fingerprint-updates-1-connected-1" but there were no matches

I spent some time analyzing how they work so I could adjust them but at this point I'm no longer sure if it's the tests that have invalid expectations or if the change actually breaks something for certain cases. I will try to get back to this topic later but if you have any pointers or ideas as the person who wrote the code, I'd very much appreciate them :)

[d4rky-pl]

Note: the proposal above is incomplete, it will crash with undefined method 'empty?' for nil if there's an error in the API implementation that causes only errors to be populated