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

Updates to work with App Sync Subscriptions #1919

Merged
merged 11 commits into from
Aug 26, 2021

Conversation

cmcnally-beachbody
Copy link
Contributor

@cmcnally-beachbody cmcnally-beachbody commented Aug 24, 2021

Task
Closes #1837

Original Discussion - https://github.com/apollographql/apollo-ios/discussions/1820

Summary

  • Update to allow for creating custom operation message ids for subscriptions
  • Update to writeQueue on start_ack message

@apollo-cla
Copy link

@cmcnally-beachbody: 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/

@cmcnally-beachbody
Copy link
Contributor Author

@designatednerd Here is the update we discussed about two months ago to handle app sync subscriptions

@calvincestari calvincestari self-assigned this Aug 24, 2021
@designatednerd
Copy link
Contributor

I'm at a conference but @calvincestari will be taking a look!

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the PR! I'll be honest, I don't know a lot about what this operation id is used for and why we need this PR. Can you explain a little bit about what this PR is trying to do so we can better review it please?

I'm thinking we might want to add some more documentation to this for the future.

Tests/ApolloTests/OperationMessageIdCreatorTests.swift Outdated Show resolved Hide resolved
Sources/ApolloWebSocket/WebSocketTransport.swift Outdated Show resolved Hide resolved
Sources/Apollo/OperationMessageIdCreator.swift Outdated Show resolved Hide resolved
@cmcnally-beachbody
Copy link
Contributor Author

Thanks so much for the PR! I'll be honest, I don't know a lot about what this operation id is used for and why we need this PR. Can you explain a little bit about what this PR is trying to do so we can better review it please?

I'm thinking we might want to add some more documentation to this for the future.

I think this discussion we had started pretty much sums up the why behind needing this change. In our use case we will just be generating UUID strings as the identifiers - #1837

@calvincestari
Copy link
Member

calvincestari commented Aug 25, 2021

@cmcnally-beachbody - it looks like you and @johnclayton both work at the same place and have alignment on this. I want to make sure this solution works for the original request too if that's not the case.

@cmcnally-beachbody
Copy link
Contributor Author

@cmcnally-beachbody - it looks like you and @johnclayton both work at the same place and have alignment on this. I want to make sure this solution works for the original request too if that's not the case.

Yea we are good, I was just taking the original request over the line and we have just been using our own forked version for a bit.

@calvincestari calvincestari merged commit 550f1b1 into apollographql:main Aug 26, 2021
@calvincestari
Copy link
Member

calvincestari commented Aug 26, 2021

Thanks for the contribution @cmcnally-beachbody and @johnclayton! This will go out with the next release, maybe only next week though as there is another PR we need to get out in that release too.

@cmcnally-beachbody
Copy link
Contributor Author

Thank you @calvincestari and appreciate the help

@calvincestari calvincestari mentioned this pull request Aug 31, 2021
6 tasks
@calvincestari
Copy link
Member

calvincestari commented Feb 22, 2022

@cmcnally-beachbody I have a favour to ask if you have the time. I'm working on #2168 which adds support for the graphql-ws protocol and while I was doing that I 'formalized' our support for AppSync. There is no functional change in how the protocol communication behaves for AppSync but if you have the time would you mind pulling that branch into your code and running it through any tests you have for that part of the code. We have integration tests for subscriptionWsProtocol and graphqlWsProtocol but nothing for AppSync.

WSProtocol cases are named according to the protocol not the implementation so while is no named support of AWS AppSync specifically it implements the graphql-ws protocol, with the addition of start_ack which we handle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to send custom identifiers with web socket messages
7 participants