subscribeToMore unsubscribes after partial error

[skolodyazhnyy]

Issue Description

When subscribeToMore query fails partially, client would unsubscribe and call onError handler.

When I say "fails partially", I mean top node resolves but some nested nodes return an error. Server returns both data with partial data and errors with an array of errors.

Unfortunately subscribeToMore does not provide errorPolicy nor any other mechanism to handle partial response. It seems if any node in the response returns an error the whole subscription is considered to "fail".

Link to Reproduction

Description is enough

Reproduction Steps

Here is an example message exchange between client and server,

Client subscribes to a onChange subscription, querying two fields ok and ko

{
    "id":"1",
    "type":"subscribe",
    "payload":{"variables":{},"operationName":"X","query":"subscription X { onChange { ok ko } }"}
}

In response, field ok resolves to "hello", but ko resolver returns an error.

{
    "payload":{
        "errors":[{"message":"error message","path":["onChange","ko"],"extensions":{}}],
        "data":{"onChange":{"ok": "hello"}},
    }
    "id":"1",
    "type":"next"
}

I would like to handle partial response, and deal with error separately, but instead updateQuery never gets called and subscription completes.

subscribeToMore({
    onError: (err) => {
        console.log('err', err); // this gets executed
    },
    document: WATCH_CHANGE,
    updateQuery: (prev, { subscriptionData }) => {
        console.log('subscriptionData', subscriptionData); // does not get executed at all
    },
}),

@apollo/client version

3.9.6

[skolodyazhnyy]

It actually seems to be pretty trivial, it seems it's possible to pass errorPolicy (and maybe fetchPolicy too) here https://github.com/apollographql/apollo-client/blob/8bc7d4d406402962bf5151cdd2c5c75c9398d10c/src/core/ObservableQuery.ts#L570

[jerelmiller]

Hey @skolodyazhnyy 👋

You're right, seems like we should be able to forward those options along to startGraphQLSubscription. I think this is likely a good change.

I'm also wondering if it would make sense to default those values to the same as the query itself. For example, if you set errorPolicy to all in your query, perhaps subscribeToMore should also default to this value. What are your thoughts on that?

I'll take this to our next team meeting as well to discuss this idea to make sure I'm not thinking of this the wrong way. We'll be releasing 3.10 this week so no guarantees on a timeline here, but we will likely try and look at this for a 3.10.x patch release. Thanks again for the report!

[skolodyazhnyy]

Thanks @jerelmiller.

I think inheriting values makes sense, subscription queries are normally follow same "logic" as their main query. If developer already wrote error handling logic for main query it will work for follow up updates.

Another thing to keep in mind, is making sure errors can be handled in a convinient way.

I think they probably should be available in the updateQuery as you may want to modify how update is merged with original query depending on what errors you are getting.

I'm not sure if onError should be called, I think it will be backwards incompatible change as current call to onError means "subscription is over".

And, since I have you here, it would have been nice to add onComplete option to subscribeToMore so I can get notified when subscription has completed successfully :)