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

WebSocket Fixes - Revert to Starscream 3.x and invert dependency #1861

Merged
merged 10 commits into from
Jul 6, 2021
Merged

WebSocket Fixes - Revert to Starscream 3.x and invert dependency #1861

merged 10 commits into from
Jul 6, 2021

Conversation

AnthonyMDev
Copy link
Contributor

@AnthonyMDev AnthonyMDev commented Jul 6, 2021 •
edited
Loading

Changelog Additions

  • Breaking - Downgraded from Starscream v4 to v3! After upgrading to Starscream 4.0, a lot of our users started to experience crashes while using web sockets. We've decided to revert to the more stable Starscream version 3. In order to fix a few known bugs in Starscream 3, we have made a fork of Starscream that Apollo will depend on going forward.
    In preparation for moving to Apple WebSockets in the future, we have also fully inverted the dependency on Starscream. Between these two changes, a lot of breaking changes to our Web Socket API have been made:
    • The ApolloWebSocketClient protocol was removed and replaced with WebSocketClient.
    • WebSocketClient does not rely directly on Starscream anymore and has been streamlined for easier conformance.
    • ApolloWebSocket, the default implementation of the WebSocketClient has been replaced with DefaultWebSocket. This implementation uses Starscream, but implementations using other websocket libraries can now be created and used with no need for Starscream.
    • WebSocketClientDelegate replaces direct dependency on Starscream.WebSocketDelegate for delegates.

@AnthonyMDev AnthonyMDev added the include-in-changelog Indicates that changes from a PR should be noted in the changelog for their release. label Jul 6, 2021
@AnthonyMDev AnthonyMDev added this to the July 2021 milestone Jul 6, 2021
@AnthonyMDev AnthonyMDev mentioned this pull request Jul 6, 2021
Closed
@calvincestari calvincestari self-requested a review July 6, 2021 19:37
@AnthonyMDev
Copy link
Contributor Author

@designatednerd Do you know if there are any changes to the docs that need to happen for this before we cut a new release? Want to cut a release probably tomorrow, so I'd like to get that squared away.

calvincestari
calvincestari approved these changes Jul 6, 2021
View reviewed changes
Package.swift Show resolved Hide resolved
Sources/ApolloWebSocket/DefaultWebSocketClient.swift Outdated Show resolved Hide resolved
Package.resolved
"revision": "f14ff47f45642aa5703900980b014c2e9394b6e5",
"version": "0.9.0"
"revision": "f79d4ecbf8bc4e1579fbd86c3e1d652fb6876c53",
"version": "0.9.2"
Copy link
Member

Choose a reason for hiding this comment

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

I generally don't think we should be pulling other dependency updates into focused PRs. If we ever wanted to revert this PR it ends up affecting other things too. I'm not requiring a change, will approve

designatednerd
designatednerd reviewed Jul 6, 2021
View reviewed changes
Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

In the notes: we have made a private forked of Starscream - I think that should probably be we have made a fork of Starscream, since the fork isn't actually private.

Apollo.xcodeproj/project.pbxproj
@@ -3807,7 +3812,7 @@
repositoryURL = "https://github.com/daltoniam/Starscream.git";
requirement = {
kind = upToNextMinorVersion;
minimumVersion = 4.0.4;
minimumVersion = 3.1.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there's some kind of reference to the main Starscream repo floating around in here

Apollo.xcodeproj/project.pbxproj
isa = XCSwiftPackageProductDependency;
package = DE8C84F2268BBF8000C54D02 /* XCRemoteSwiftPackageReference "Starscream" */;
productName = Starscream;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

also not clear on why this has been added like three times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not clear on that either... XCProject files still have some weird bugs with their SPM integrations sometimes. I'm not too worried about it though. Not sure if there is anything we can do to fix that...

@AnthonyMDev AnthonyMDev merged commit d991cb7 into main Jul 6, 2021
@AnthonyMDev AnthonyMDev deleted the starscream-revert branch July 6, 2021 21:10
@hwillson hwillson removed this from the MM-2021-07 milestone Jul 29, 2021
@mdubs
Copy link

mdubs commented Aug 12, 2021

Is there any plan to remove the actual Starscream dependency being declared in the main Apollo Package.swift? Currently even though we don't use the ApolloWebsocket target, SPM/Xcode will clone the Starscream dependency (due to https://github.com/apple/swift-evolution/blob/main/proposals/0226-package-manager-target-based-dep-resolution.md not being fully implemented). This means that we are still prevented from using Starscream 4.x (or really any version except for the Apollo forked 3.1.2 version).

@AnthonyMDev
Copy link
Contributor Author

I will fix this tomorrow.

@AnthonyMDev
Copy link
Contributor Author

So l'm starting to look into this. Moving the Starscream dependency into another target (out of ApolloWebSocket) makes sense, but moving it out of the Package.swift file altogether seems messy and makes integration for others require additional steps. I'm going to talk to the team today about how we want to approach this.

@AnthonyMDev
Copy link
Contributor Author

#1906 should fix this for you @mdubs! Will put a release out next week.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@designatednerd designatednerd designatednerd left review comments

@calvincestari calvincestari calvincestari approved these changes

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.
5 participants
@AnthonyMDev @mdubs @calvincestari @designatednerd @hwillson