(feat): Sync Streams #35
Conversation
…ler logic to StreamingSyncImplementation.
simolus3
left a comment
simolus3
left a comment
There was a problem hiding this comment.
Thanks for taking a look at sync streams 🚀 I have a few high-level / API design comments from a quick look, but please treat them as optional since I'm not that familiar with the dotnet SDK.
…ia a factory. Scaffolding A MockRemote test.
simolus3
left a comment
simolus3
left a comment
There was a problem hiding this comment.
This looks good to me from a high-level perspective (since I don't know C# that much).
| [JsonProperty("streams")] | ||
| public object[]? Streams { get; set; } = []; |
There was a problem hiding this comment.
Out of interest, why did this have to be added here? Since we're using the Rust client, I'm kind of surprised we need these protocol message types at all.
If they're only used for mock tests, maybe we should move them to test sources (but it probably makes sense to do that in a separate PR).
There was a problem hiding this comment.
Yes this just made some of the testing easier, and yes - open to moving it in the future.
LucDeCaf
left a comment
LucDeCaf
left a comment
There was a problem hiding this comment.
Happy overall, might be able to improve the user-facing API a little bit.
| public interface IPowerSyncDatabase : IEventStream<PowerSyncDBEvent> | ||
| { | ||
| public Task Connect(IPowerSyncBackendConnector connector, PowerSyncConnectionOptions? options = null); | ||
| public ISyncStream SyncStream(string name, Dictionary<string, object>? parameters = null); |
There was a problem hiding this comment.
I think it would be nice for users if they could pass an anonymous object which is serialised later. Something like this:
powerSync.SyncStream("stream", new { foo: "a" }).Subscribe();It would also be nice for the SDK to support the Dictionary<string, object> syntax so that users can build up a list of parameters iteratively, so if we support new { ... } we shouldn't drop support for new Dictionary<string, object>() { ... }. I think both Newtonsoft.Json and System.Text.Json handle both cases just fine.
There was a problem hiding this comment.
I agree, we can update this alongside client params in a follow up.
Chriztiaan
force-pushed
the
feat/sync-streams
branch
from
361396c to
9d9bd22
Compare
Added support for sync streams, based on reference implementations from other SDKs.
Minor changes were made to the API to support mocking of the remote, this allows new possibilities with mocking the backend when testing.