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

Pass protocols to WebSocket constructor in JSWebSocketEngine #4445

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

baconz
Copy link
Contributor

@baconz baconz commented Oct 7, 2022

Of note, subscription-transport-ws (I know it is old...) will close any connection that does not include the protocol header.

See: https://github.com/apollographql/subscriptions-transport-ws/blob/master/src/server.ts#L132

@BoD called this out here: #4339

Of note, subscription-transport-ws (I know it is old...) will close
any connection that does not include the protocol header.
@netlify
Copy link

netlify bot commented Oct 7, 2022

👷 Deploy request for apollo-android-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 9f55dc6

@baconz baconz changed the title Pass protocols to WebSocket constructor in JSWebSocketEngine Pass protocols to WebSocket constructor in JSWebSocketEngine Oct 7, 2022
@BoD
Copy link
Contributor

BoD commented Oct 7, 2022

Thanks! 🙏 Looks good to me. Just to be sure, as we don't have many tests on the WebSocket+JS side, did you get hit by this on your project and that fixes it?

@baconz
Copy link
Contributor Author

baconz commented Oct 8, 2022

Well, it wasn't working!! It was tough to get this tested because I can't get the project to build on apple silicon. It seems this is a known issue in Gradle-land and they're working on it (gradle/gradle#19140).

Anyway, I got it to build on a Linux box and tested it out. Should all be working with my latest patch.

@BoD
Copy link
Contributor

BoD commented Oct 10, 2022

Well, it wasn't working!!

Thanks a lot for checking! 🙏

I can't get the project to build on apple silicon

Oh interesting, we're working on Apple silicon too but I don't think we've tried the toolchain auto-provisioning feature, and just installed a compatible JDK manually.

The fix looks good.

@BoD BoD merged commit 972b8cf into apollographql:main Oct 13, 2022
@BoD
Copy link
Contributor

BoD commented Oct 13, 2022

Sorry for the delay! This will be in the next release, and can be also tried out with the snapshots.

@baconz
Copy link
Contributor Author

baconz commented Oct 14, 2022

Sweet thanks for merging! I tried out the snapshot and things are working!

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.

2 participants