Skip to content

Conversation

@Chriztiaan
Copy link
Collaborator

No description provided.

Copy link

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

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.

/// The TTL controls when a stream gets evicted after not having an active <see cref="ISyncStreamSubscription"/> object
/// attached to it.
/// </summary>
public int? Ttl { get; set; }

Choose a reason for hiding this comment

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

I'm not too familiar with dotnet, but generally I prefer to use the canonical duration type of the language (e.g. we're using a Duration for this in Dart). Does TimeSpan work?

Comment on lines +90 to +96
public enum SyncPriority
{
Priority_0 = 0,
Priority_1 = 1,
Priority_2 = 2,
Priority_3 = 3
}

Choose a reason for hiding this comment

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

We might introduce more priorities in the future, so exposing this as an enum would be breaking then. See e.g. the Dart type we use for this.

LLM translation that looks reasonable to me:

public readonly record struct StreamPriority : IComparable<StreamPriority>
{
    private const int Highest = 0;

    public int PriorityNumber { get; }

    public StreamPriority(int priorityNumber)
    {
        if (priorityNumber < Highest)
            throw new ArgumentOutOfRangeException(
                nameof(priorityNumber),
                "Priority must be >= 0");

        PriorityNumber = priorityNumber;
    }

    // Higher priority = larger number
    public int CompareTo(StreamPriority other) =>
        other.PriorityNumber.CompareTo(PriorityNumber);

    /// <summary>
    /// The priority used by PowerSync to indicate that a full sync was completed.
    /// </summary>
    public static readonly StreamPriority FullSyncPriority =
        new(int.MaxValue);
}

return null;
}

var streamParamsJson = JsonConvert.SerializeObject(stream.Parameters);

Choose a reason for hiding this comment

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

It would be nice to either cache this serialization result on the sync description or to perhaps implement an equals check for maps (I don't know if C# has something similar to package:collection's MapEquality).

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.

3 participants