Allows for subscriptions to be de-duped

[jkossis]

Allows for the consumer of the subscription to specify whether there should be de-duplication or not. This keeps the backwards compatible behavior of false, so it shouldn't introduce any kind of breaking behavior.

[apollo-cla]

@jkossis: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[jkossis]

We also could do something similar to here (where it is omitted)

apollo-client/src/core/QueryManager.ts

Lines 771 to 775 in e440b49

	this.getObservableFromLink(
	queryInfo.document!,
	options.context,
	options.variables,
	),

and defer the dedup to getObservableFromLink's default assignment.

[jkossis]

@benjamn what are your thoughts? I saw your comments here (apollographql/apollo-feature-requests#74), and it seemed like you were in favor of this behavior. Interested in your input.

[benjamn]

@jkossis I would have thought this should already work (unless there's a more general problem), since we are passing context from startGraphQLSubscription to getObservableFromLink, and getObservableFromLink uses the context parameter to compute a default value for the deduplication parameter:

    deduplication: boolean =
      // Prefer context.queryDeduplication if specified.
      context?.queryDeduplication ??
      this.queryDeduplication,

That said, I can totally imagine there being issues with deduplicating subscriptions, since they last a lot longer than queries, and have more results, so I'm interested to hear what kind of (mis)behavior you're seeing.

[jkossis]

@benjamn I agree, the context is already being looked at in getObservableFromLink. However, that default assignment is never being executed as false is being explicitly passed in the case of a subscription.

IMO, the most flexible solution may be omitting that 4th parameter when calling getObservableFromLink in the case of a subscription (similar to query), and allowing the default assignment to take place. I can make that change if you concur.

In terms of misbehavior, false being hard-coded requires de-duping to be managed in the consuming app's code, when Apollo has the structures and mechanisms in place to do this quite nicely already. I tested it out with a sample Hasura project, and it works wonderfully. For example, if N components are interested in using a query that has an associated subscribeToMore, only one message is sent over webscoket, not N messages.

I think the de-duping may have less issues than anticipated. The logic would be if an identical subscription is active, don't create a new one. And I think this may already work correctly as is. But you are significantly more familiar with the code, so I lean on you.

[jkossis]

@benjamn thoughts on above? I think the best solution is not passing the 4th parameter to getObservableFromLink, and letting the default assignment do its work.

[jkossis]

@hwillson any thoughts as well? Seems like this could be a nice performance gain essentially for free.

[ntziolis]

Is there anything I can do to help this PR get merged in?

[rapacity]

any chance we'll be able to get de-duplicated subscriptions? this would really improve performance

it just needs a really small change to allow this option to be left up to the user rather than how it is right now, forced to be false

[carlosdp]

would really love for this to be merged in as well

[ntziolis]

again happy to provide assistance in any form. what is keeping this change from being merged in?

[jkossis]

Just waiting on someone with write access to merge this in. Seems to be an easy fix, that is backwards-compatible.

[ntziolis]

@jkossis Has anyone with write access so far been involved in this thread?

If not:
Is the subscription deduplication configurable on/off? If not I would recommend the PR to be extended to support this to be configurable (with default to off). I could image that the maintainers are concerned that such a change could impact many existing users. I'm happy to assist just let me know.

[jkossis]

@ntziolis I believe @benjamn has write access.

In terms of configuration, this change actually allows for greater configuration, leveraging the assignment here:

apollo-client/src/core/QueryManager.ts

Line 774 in 3d3e77b

deduplication: boolean =

Right now, false is being passed explicitly, so you can't configure it on the request's context, or at the global level (this.queryDeduplication).

[ntziolis]

@ntziolis I believe @benjamn has write access.
Perfect.

Right now, false is being passed explicitly, so you can't configure it on the request's context, or at the global level (this.queryDeduplication).

I see. I would have thought this global level config might have been an issue since it will effect existing users, but after following the linked threads it seems that the apollo team is ok with this behavior change as you pointed out.

Thank you for pushing this forward.

@benjamn @hwillson Is there anything keeping the apollo team from getting this merged in?

[carlosdp]

Is there anything blocking this? It's a one line change that saves a ton of network traffic and unnecessary overhead.

[floriaanpost]

It would be great if this could be fixed! I never changed my code, because I hoped this would be merged in soon. Will this still be fixed, or should I make some workaround? Thanks for all the great work with Apollo!

[benjamn]

All the updates/reasoning here since I last looked at this issue make sense to me! Thanks for continuing to work on this @jkossis.

[benjamn]

@jkossis @floriaanpost @ntziolis @carlosdp @rapacity These changes are now available in @apollo/[email protected] (just published). Please take a look / give it a try. We can backport these changes from release-3.4 to main (3.3.x) if need be, once they've been proven.

[KeithGillette]

I take it this PR solves the question I raised in discussion #7360?