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

Dedup subscriptions #74

Closed
thomassuckow opened this issue Dec 6, 2018 · 13 comments
Closed

Dedup subscriptions #74

thomassuckow opened this issue Dec 6, 2018 · 13 comments
Assignees
Labels
project-apollo-client (legacy) LEGACY TAG DO NOT USE

Comments

@thomassuckow
Copy link

When multiple parts of the UI utilize the same subscription it will be subscribed to for each occurance.

This increases latency to the user because it doesn't pull from the cache and increases network traffic due to duplicated payloads.

There already appears to be a start at deduplicating in QueryManager, it utilizes an observables array despite it always containing 0 or 1 observable.

@lorensr
Copy link

lorensr commented Jan 25, 2019

For example, I have a <Subscription subscription={ON_NEW_CHAT}> in NewMessageNotification.js and a subscribeToMore({ document: ON_NEW_CHAT }) in Channel.js. I see in the websocket traffic that this creates two identical subscriptions.

@benjamn
Copy link
Member

benjamn commented Jun 6, 2019

For the record, the only thing currently stopping this from working is that we explicitly disable deduplication for subscriptions by passing false for the deduplication parameter of getObservableFromLink here.

@benjamn benjamn added the project-apollo-client (legacy) LEGACY TAG DO NOT USE label Jun 6, 2019
@thomassuckow
Copy link
Author

Is there any reason it can't be true?

@benjamn
Copy link
Member

benjamn commented Jun 7, 2019

@hwillson Any thoughts on deduplicating subscriptions by default?

@benjamn benjamn self-assigned this Jun 7, 2019
@hwillson
Copy link
Member

@benjamn I'm all for it (and we should be safe to deduplicate subscriptions in a minor/patch release).

@qeternity
Copy link

Any word on this release? What's the current best practice to have multiple components received subscription events?

@jpikora
Copy link

jpikora commented Sep 12, 2019

@qeternity I believe that ppl are currently moving subscriptions from child components to common parents. It sort of works, but is far from ideal since you end up having complex detached logic in parent components, just to update queries used in child components.

It would be much more pleasant to handle those updates directly in child components, letting us reuse same subscriptions multiple times which should be deduplicated by Apollo automatically.

If it's just a matter of setting an internal flag, would be great to have this enabled.

I believe this would improve traffic for a lot of apps using Apollo subscriptions, since not everyone took the road of abstracting subscriptions to higher levels.

@dmaskasky
Copy link

@hwillson @benjamn @jpikora

I am composing useSubscription into a custom react hook. For each call position where the hook is used, a new subscription is created. This deduplication issue is compounded with the addition of @apollo/react-hooks.

@jkossis
Copy link

jkossis commented Jun 15, 2020

Any movement on setting this to true? Would really help performance!

@thomassuckow
Copy link
Author

thomassuckow commented Jun 15, 2020

QueryManager has evolved quite a bit. I do now see a constructor parameter, though I am not sure what it takes to set it. https://github.com/apollographql/apollo-client/blob/8c1d8b4dc5ac964dd791bf635918309aa1474b70/src/core/QueryManager.ts#L88

Personally I'm not in a position to dive too deep into this at the moment since client 3 is not playing nice with our app so we are trying to mitigate that first. The GraphQL spec is so bloody complex, I'm tempted again to move to making a graphql-lite in yaml without aliases or type enforcement.

@ntziolis
Copy link

The PR #6910 that enables this is ready and waiting for approval / comments since quite a while, any reason this hasn't been merged in.

@TiagoJacobs
Copy link

I see this PR was merged, should this issue be closed?

@jerelmiller
Copy link
Member

Completed with apollographql/apollo-client#6910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project-apollo-client (legacy) LEGACY TAG DO NOT USE
Projects
None yet
Development

No branches or pull requests