realm-js icon indicating copy to clipboard operation
realm-js copied to clipboard

Add support for counters

Open kneth opened this issue 4 years ago • 19 comments

Problem

Realm Core supports a special case of integers: counters. Counters are in particular useful for synced Realms.

See also https://github.com/realm/realm-core/issues/5056

Solution

We'll be adding a new property type named "counter", which will use the "int" core type underneath but return instances of a Counter class instead of raw number.

declare class Counter {
  public get value(): number;
  public increment(by = 1);
  public decrement(by = 1);
  public set(value: number);

  // Implements https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/valueOf
  // this would allow using an instance of `Counter` to be coerced into a `number`.
  public valueOf(): number {
    return this.value;
  }
}

Because we cannot overload the increment operators ++, +=, -- and -= explicitly in JS (these simply call "set" / assignment underneath), we suggest throwing if these are used, to prevent users from relying on these operators which wouldn't use the proper counter semantics.

We discussed implementing a heuristic (if the setter is invoked just after the getter, perhaps users are performing a +=), but we wouldn't be able to distinguish that from a *= 2, in which case it would be misleading to use the counter semantics.

How important is this improvement for you?

No response

kneth avatar Nov 19 '21 09:11 kneth

I wonder what an intuitive API would look like.

We cannot overload the ++ or += operators (because that's a limit in JS), and as I see it we have (at least) two options:

  1. Calculate the difference to the existing value when performing an assignment (which is the underlying primitive called when users perform ++ or += on a Number) and use this to call into core to perform the increment. We should consider exposing this behind a flag on the "property schema" passed when opening the Realm, to allow users to opt into this behavior, per property.
  2. New increment and decrement instance methods on Realm.Object, taking the property name and an optional value to increment by (defaulting to 1):
    car.increment("passengers", 2); // Two people are stepping in
    car.decrement("passengers"); // One person is stepping out
    
    One drawback of this approach is that it interfeer with the property namespace owned by the user through the schema (i.e. users won't be able to use the feature and have properties named "increment" or "decrement" at the same time).

kraenhansen avatar Apr 11 '24 07:04 kraenhansen

  1. I think this is the most intuitive, but definitely needs to be something the user opts into. The question on this is how to opt-in? One way would be to define a new data type and have that wrap "int". Or we would add an option to the property object.

  2. I assume this would be easier to implement, but not as pretty as option 1. Alternatively, the increment/decrement methods could be a static function in the Realm namespace (or Realm.Object) that one could call, to avoid adding functions to the users defined object.

takameyer avatar Apr 11 '24 07:04 takameyer

One way would be to define a new data type and have that wrap "int". Or we would add an option to the property object.

I agree - it seems we have (at least) two alternative ways of declaring the "counter" behavior on an int:

1.a type: "counter"

A new counter type that would use int on the core level but use the Obj#add_int API when performing assignments.

// shorthand form
schema: [{
  name: "Car",
  properties: {
    passengers: "counter"
  }
}],
// long form
schema: [{
  name: "Car",
  properties: {
    passengers: {
      type: "counter",
      indexed: true,
    }
  }
}]

Pros:

  • Would probably be easiest to implement.
  • Perhaps more discoverable as it could get its own item on lists of supported types in documentation.

Cons:

  • Could cause confusion, since it would introduce a new type which other SDKs don't have. Although other SDKs would be able to declare and interact with these values through an int.
  • This takes precedence over user-defined model names and is potentially a breaking change.

1.b) counter: true

This which can be set on the property schema of a type: "int" property:

schema: [{
  name: "Car",
  properties: {
    passengers: {
      type: "int",
      indexed: true,
      counter: true, // would throw if used on types other than `int`
    }
  }
}]

Pros:

  • It feels more like a behaviour of the "int"
  • it could anticipate a future where we'd like to add more orthogonal behaviour.

Cons:

  • Doesn't support a shorthand notation.

1.c) presentation: "counter"

This which can be set on the property schema of a type: "int" property:

// shorthand form
schema: [{
  name: "Car",
  properties: {
    passengers: "counter"
  }
}],
// long form
schema: [{
  name: "Car",
  properties: {
    passengers: {
      type: "int",
      presentation: "counter", // would throw if used on types other than `int`
    }
  }
}]

Pros:

  • It feels more like a behaviour of the "int"
  • it could anticipate a future where we'd like to add alternative presentations to other types (such as "record" and "map" for dictionaries).

I'm probably leaning slightly towards 1.c.

kraenhansen avatar Apr 11 '24 07:04 kraenhansen

I think 1.a would be best as well. The other SDKs have different ways of codifying the counter. I could actually only find docs in Kotlin (which implements a separate type) and .NET.

The user will probably not be confused, since they won't be using multiple SDKs. IMO best to just focus on what is most intuitive for the platform.

takameyer avatar Apr 11 '24 09:04 takameyer

We will also need to add some good documentation around this. It should be clear that simply assigning a number to a counter will cause all previous increments and decrements. Assignment essentially resets the counter to a value. So the API for a counter would be:

  • = assignment that sets the value directly (when syncing this assignment will ignore previous decrement and increment operations)
  • -= and -- decrement the counter
  • += and ++ increments the counter

takameyer avatar Apr 11 '24 09:04 takameyer

As I hinted in my first comment, we don't have ways to explicitly intercept increments and decrements (++, --, += and -=) except through a setter, which would also be hit when doing regular assignments.

We might be able to apply a heuristic which would hook into the getter and detect an increment / decrement if the getter is accessed just before the setter is called, in the same synchronous execution block 🤔 I'm uncertain if this would have any drawbacks or be too weird of an API, but I can't really think of any other ways to achieve increments, decrements and explicit assignment, through operators.

kraenhansen avatar Apr 11 '24 10:04 kraenhansen

Can we have this return a custom type, similarly to what we do in C#? If we could, then we'd be able to define functions users can use to increment/decrement rather than the ++/-- operators.

I'm not sure if that'd work in js, but roughly something like:

class Counter {
  private objKey: ObjKey;
  private colKey: ColKey;

  public get value(): number {
    return core.getValue(this.objKey, this.colKey)
  }


  public increment(value: number) {
    core.increment(this.objKey, this.colKey, value)
  }
}

Counter.prototype.valueOf = function() {
    return this.value;
}

That way, you could still use that if you need to do print(obj.myCounter + 5), but you could also do obj.myCounter.increment(5).

nirinchev avatar Apr 11 '24 12:04 nirinchev

@nirinchev that's a cool approach and a great use of the valueOf method - the only drawback I can think of is that typeof obj.myCounter would be "object" and not "number". I like how it's more explicit (not relying on the heuristic I mentioned earlier) and that the increment method doesn't bleed into the users namespace of property names 👍

kraenhansen avatar Apr 11 '24 12:04 kraenhansen

With the class Counter approach, I imagine we need to annotate it in the schema. The approach cars.increment("passengers", 2) will not (any int property is allowed), and it will make it more flexible. Even if passengers is mixed, we can validate that it is an int before calling Obj::add_int().

kneth avatar Apr 11 '24 14:04 kneth

I also just though to of a Con for the counter type. If we are expecting a counter to be part of a mixed data type, it may difficult to determine what action to take when using ++ or +=.

takameyer avatar Apr 12 '24 08:04 takameyer

The ++ and += operators are inherently not something we want/can support due to the way they work - they roughly expand to foo = foo + 1, which is assignment rather than an increment operation. They also are not supposed to modify the underlying value, whereas with counters we want exactly that.

nirinchev avatar Apr 12 '24 12:04 nirinchev

not something we want/can support

We could probably support it, via the heuristic I outlined above - if we want to is a different question. I do see some value in that: I'd expect some users to gravitate towards that way of incrementing a counter and be surprised that it works (it goes up), but doesn't uphold the same guarantees as calling an increment() method.

They also are not supposed to modify the underlying value

Why is that? Calling car.passengers++ or car.passengers += 1 should update the value in the database?

kraenhansen avatar Apr 12 '24 12:04 kraenhansen

They also are not supposed to modify the underlying value

Typically, it's because the value that the ++ operator operates on is immutable (i.e. an integer). The reason why car.passengers++ modifies the car is because this expands to car.passengers = car.passengers + 1 - i.e. it's assigning the result of the calculation to the original variable/property. I don't believe it's possible in JS, but in languages where ++ is overloadable, it'd be bad practice to implement it in a way where it mutates the object it operates on, rather than returning a new value, which is then assigned to the variable that used to hold the object. If we had a Counter class and could overload ++, it'd be a bad practice to mutate the database at that point, but we should rather return some value, that when assigned to the original variable/property tells it to increment itself by some value. That's a bit theoretical though and not something I want to get too bogged down in.

I feel the more important discussion is whether silently replacing the behavior of ++ with calling .increment() is the right thing to do, because of the assignment semantics that the operator has. We'd be walking a fine line, trying to predict user intent with heuristics, which may backfire in obscure ways. It's also possible I don't have enough js knowledge to appreciate how smart the heuristics can be, but I can easily imagine a situation where a user does something like company.projectedRevenue = company.projectedRevenue * 1.2, which we could interpret as an increment rather than an assignment. There may be solutions out there, I'm just a bit conservative when it comes to adding magic to the SDK and would typically err on the side of doing something dumb and obvious rather than something very clever that may catch users by surprise.

nirinchev avatar Apr 12 '24 12:04 nirinchev

I don't believe it's possible in JS, but in languages where ++ is overloadable, it'd be bad practice to implement it in a way where it mutates the object it operates on

Operatores aren't overloadable, as such, but implementing proxy handlers for get + set could get us a long way towards something that behaves like it.

I think I understand you now, by "underlying value", you ment the primitive, I thought you ment the car.passengers (in my example). I agree that's not desirable for the primitive itself to update (nor is it possible in JS), as they are always copied and never accessed by reference.

a user does something like company.projectedRevenue = company.projectedRevenue * 1.2, which we could interpret as an increment rather than an assignment.

I agree - that would indeed be unfortunate 🤔

kraenhansen avatar Apr 12 '24 13:04 kraenhansen

I've updated the description above with my takeaway of the discussions above ☝️

kraenhansen avatar Apr 15 '24 12:04 kraenhansen

  1. My 2¢, from the perspective of aiming to provide an unambiguous interface where the user's assumptions match the semantics, I could see it as somewhat problematic to try to intercept get/set calls from using the operators in order to make it behave like a counter. E.g. I see it as reasonable for a user to assume that it'd act exactly like a JS Number, but also reasonable for another user to assume that it'd act like a counter, and for a third to not know what to assume. Essentially, I think allowing that would cause some discrepancy between assumption and semantics, while having distinct APIs like increment(), etc. removes that.

  2. Regarding type: "counter" vs counter: true.

    • If we ultimately decide to only support modification via specific APIs like in the current issue description, I think having type: "counter" would better distinguish this from a regular int, as the way a user would use the type would be quite different for some expressions.
  3. Data type name

    • "counter" seems appropriate assuming it would/could be used only for that purpose?
    • From the docs links @takameyer sent, it's interesting that Kotlin and .NET opted for MutableRealmInt/RealmInteger.

elle-j avatar Apr 26 '24 07:04 elle-j

Regarding the name - the reason .NET and Kotlin opted for some variation of Realm + Integer is to be aligned with other types - e.g. Realm + List is a type that behaves similarly to a BCL List, but adds some Realm API on top of it, Realm + Object is a regular object + some Realm stuff on top, so it seemed appropriate for a value that is an integer + some extra API to name it in a similar fashion. It also serves future-proofing purposes where - if necessary - we can add more functionality, that may or may not be related to counting, without breaking changes or making the API awkward.

nirinchev avatar Apr 26 '24 11:04 nirinchev

Okay yeah interesting, makes sense.

elle-j avatar Apr 26 '24 12:04 elle-j

P.S. looks like some first steps were taken about a year ago.

elle-j avatar Apr 29 '24 12:04 elle-j