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.

Sign up for GitHub

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

Jump to bottom

Publish response from the WebSocketTransport to the ApolloStore #1889

Merged
merged 6 commits into from
Aug 4, 2021
Merged

Publish response from the WebSocketTransport to the ApolloStore #1889

merged 6 commits into from
Aug 4, 2021

Conversation

tgyhlsb
Copy link
Contributor

@tgyhlsb tgyhlsb commented Aug 2, 2021 •
edited
Loading

Pull Request

Summary

When using the WebSocketTransport for subscriptions, we've noticed that our cache is never updated. In fact, there is no code inside WebSocketTransport about any store or cache.

This PR injects the ApolloStore inside WebSocketTransport and calls the same method used within CacheWriteInterceptor.

Since Apollo and ApolloWebSocket are 2 distincts products, I had to mark some methods as public.

The solution seem to fix our issue and fit our needs. But I'm happy to take any suggestions to make it part of the original repository too.

@apollo-cla
Copy link

@tgyhlsb: 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/

@tgyhlsb tgyhlsb changed the title Publish response from the WebsocketTransport to the Apollo store Publish response from the WebSocketTransport to the ApolloStore Aug 2, 2021
@AnthonyMDev
Copy link
Contributor

Thanks for the PR! I'll look into this ASAP!

@AnthonyMDev
Copy link
Contributor

So, right now the only place we are publishing results to the store is in the CacheWriteInterceptor which is part of the request chain. I like keeping the configuration and setup for this in a single place. Having multiple areas that call into the store can make maintenance more difficult.

I know that @designatednerd wanted to find a way to integrate the WebSocketTransport into the request chain. However we are actually planning on dramatically re-writing the request chain prior to shipping v1.0. But that's not going to be until later this year.

I think the long term solution is to integrate web sockets into the request chain, but in the interim, I think this solution is probably better than nothing. A lot of people have had a lot of pain points around web sockets not updating the cache.

Until we rework all of this and put a permanent fix into place, I'm inclined to accept this PR. In order to ensure we don't break things too much for people who are already implementing their own workarounds for the web sockets not updating the cache, I'd like to make this optional. (Once we refactor it for 1.0, I don't mind things breaking, as 1.0 is going to have a lot of breaking changes and people will need to migrate their codebase.)

@tgyhlsb would you be willing to update this PR with the following change:

  • Make the ApolloStore parameter optional on the WebSocketTransport initializer, with a default value of nil.
  • Only publish results to the store if the ApolloStore is passed to the WebSocketTransport.

I'm also not a huge fan of making the publish and parseResult functions public, but I think that is okay for now. We will address this when we rework the request chain.

Thanks so much for this contribution. This is going to make a LOT of people happy!

@tgyhlsb
Copy link
Contributor Author

tgyhlsb commented Aug 4, 2021 •
edited
Loading

Thanks for the feedback. 100% agree with the optional ApolloStore, I'll make the change now ;)

Should I revert the tests changes? or keep initializing the WebSocketTransport with the store there?

@AnthonyMDev
Copy link
Contributor

Thanks so much! I like having it in the tests, if it breaks anything (race conditions or something), these might help catch it?

Merging now!

@AnthonyMDev AnthonyMDev merged commit 3358410 into apollographql:main Aug 4, 2021
@AnthonyMDev AnthonyMDev added the include-in-changelog Indicates that changes from a PR should be noted in the changelog for their release. label Aug 4, 2021
@AnthonyMDev
Copy link
Contributor

Going to make some documentation additions here and then we will release a new version with this at some point in the near future.

@AnthonyMDev AnthonyMDev mentioned this pull request Aug 4, 2021
Merged
ketenshi added a commit to scorebet/apollo-ios that referenced this pull request Aug 16, 2021
@ketenshi
Merge branch 'main' into connect-0.46.0
d164ab1 
* main: (856 commits)
  Add execution tests for ApolloClient clearCache callback queue (apollographql#1901)
  Use the provided callback queue instead of the store's default. (apollographql#1904)
  chore(deps): update dependency path-parse to 1.0.7 [security] (apollographql#1899)
  Release - 0.46.0 (apollographql#1897)
  Update subscriptions tutorial to be compatible with recent changes (apollographql#1893)
  Add docs and improve merging of records from WebSockets into cache. (apollographql#1892)
  Publish response from the `WebSocketTransport` to the `ApolloStore` (apollographql#1889)
  fix(deps): update dependency gatsby-theme-apollo-docs to v4.7.14
  Removing Swift codegen (v1) (apollographql#1873)
  Update toolchain versions used by circleci (apollographql#1875)
  fix(deps): update dependency gatsby-theme-apollo-docs to v4.7.13
  Community Updates - ROADMAP/README (apollographql#1867)
  [Release] - 0.45.0 (apollographql#1862)
  WebSocket Fixes - Revert to Starscream 3.x and invert dependency (apollographql#1861)
  Docs/discussions_2_community branch changes (apollographql#1858)
  Replace spectrum with Discourse (apollographql#1857)
  fix(deps): update dependency gatsby-theme-apollo-docs to v4.7.12
  Configure Renovate (apollographql#1854)
  Revert "Reconfiguring renovate 2/2"
  Reconfiguring renovate 2/2
  ...

# Conflicts:
#	Sources/Apollo/GraphQLQueryWatcher.swift
#	Sources/ApolloWebSocket/WebSocketTransport.swift
@ketenshi ketenshi mentioned this pull request Aug 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
include-in-changelog Indicates that changes from a PR should be noted in the changelog for their release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Query watcher not being called when cache is updated on an element by another query/subscription/mutation
4 participants
@tgyhlsb @apollo-cla @AnthonyMDev @calvincestari