Skip to content

feat: default to newClientImplementation #97

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

Draft
stevensJourney wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

feat: default to newClientImplementation #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e20c40
feat: default newClientImplementation
stevensJourney Dec 12, 2025
7a89124
deprecate the old client
stevensJourney Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Changelog

## 1.8.0 (unreleased)

* Update Kotlin SDK to 1.7.0.
* Update PowerSync SQLite core to 0.4.10
* Add the `soft` flag to `disconnectAndClear()` which keeps an internal copy of synced data in the database, allowing faster re-sync if a compatible token is used in the next connect() call
* Enable the `newClientImplementation` by default. This should improve performance and memory usage.


## 1.7.0

* Update Kotlin SDK to 1.7.0.
Expand Down
14 changes: 9 additions & 5 deletions Sources/PowerSync/Protocol/PowerSyncDatabaseProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,21 @@ public struct ConnectOptions: Sendable {
self.crudThrottle = crudThrottle
self.retryDelay = retryDelay
self.params = params
newClientImplementation = false
newClientImplementation = true
self.clientConfiguration = clientConfiguration
}

/// Initializes a ``ConnectOptions`` instance with optional values, including experimental options.
@_spi(PowerSyncExperimental)
@available(
*,
deprecated,
message: "Specifying the newClientImplementation flag is no longer needed. It is now enabled by default. The use of the old client is deprecated and will be removed in a future version."
)
public init(
crudThrottle: TimeInterval = 1,
retryDelay: TimeInterval = 5,
params: JsonParam = [:],
newClientImplementation: Bool = false,
newClientImplementation: Bool = true,
clientConfiguration: SyncClientConfiguration? = nil
) {
self.crudThrottle = crudThrottle
Expand Down Expand Up @@ -311,11 +315,11 @@ public extension PowerSyncDatabaseProtocol {
func disconnectAndClear() async throws {
try await disconnectAndClear(clearLocal: true, soft: false)
}

func disconnectAndClear(clearLocal: Bool) async throws {
try await disconnectAndClear(clearLocal: clearLocal, soft: false)
}

func disconnectAndClear(soft: Bool) async throws {
try await disconnectAndClear(clearLocal: true, soft: soft)
}
Expand Down
5 changes: 5 additions & 0 deletions Tests/PowerSyncTests/ConnectTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ final class ConnectTests: XCTestCase {
try await database.connect(
connector: TestConnector(url: fakeUrl),
options: ConnectOptions(
/// Note that currently, HTTP logs are only supported with the old client implementation
/// which uses HTTP streams.
/// The new client implementation uses a WebSocket connection instead.
/// Which we don't get logs for currently.
newClientImplementation: false,
clientConfiguration: SyncClientConfiguration(
requestLogger: SyncRequestLoggerConfiguration(
Copy link
Contributor Author

@stevensJourney stevensJourney Dec 12, 2025

Choose a reason for hiding this comment

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

Unfortunately, SyncRequestLoggerConfiguration is not marked as experimental. This could be considered a breaking change.
We should do some more investigation to see if we could possibly support logs with WebSockets.

Copy link
Contributor

@simolus3 simolus3 Dec 12, 2025

Choose a reason for hiding this comment

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

That is a good point, but I think in practice logging with WebSockets is tough since everything is in binary anyway. We probably need something custom here (even if we get the ktor Logging plugin to log for WebSockets, we'd log RSocket frames which is probably not that useful).

We could also consider an option on the Rust sync client to emit logs for lines it has received, since it has a bson decoder and a serializable IR for sync lines.

Copy link
Contributor Author

@stevensJourney stevensJourney Dec 12, 2025

Choose a reason for hiding this comment

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

I believe the logging should currently only be used for the /sync/stream and write-checkpoint routes.

IIRC the standard logging does not expose the server sent events in realtime for the HTTP method. The main options were:

  • info Which includes the URL and method of the request
  • headers the info above plus the requests headers
  • body which is the entire body (I recently discovered this) - not on a per message level, only logs once the request is completed, for the /sync/stream method this is essentially a memory hog waiting to happen.

Having similar functionality - or even more improved functionality when it comes to the body - would be nice for WebSockets. I agree, due to the differences in the implementation, I imagine we'd need to implement our own layer for this.

The main question is if we should delay the switching of the default implementation until we implement this (if we decide to implement it).

I get the feeling that a strong notice for the change in functionality should outweigh the potentially large memory usage of the old implementation.

Copy link
Contributor

@simolus3 simolus3 Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I get the feeling that a strong notice for the change in functionality should outweigh the potentially large memory usage of the old implementation.

I agree with this, we should clearly mention this in the changelog and release notes.

requestLevel: .all
Expand Down