RFE: Expose clientID on Read/WriteTransaction

[aboodman]

Many mutators in multiplayer apps end up taking the clientID as a parameter. It would be nice if it was implicitly available.

This would also allow for a nice symmetry with the subscription functions, which can already be written to rely on the clientID implicitly, because they only run on client side and can therefore grab it out of some other state.

[aboodman]

@arv - agreed? If so can we add fixit to this one.

[arv]

Easy to do. If you say it has high value lets do it.

[aboodman]

OK specific API proposal:

interface ReadTransaction {
+ readonly clientID string;
}

:-).

We could have a discussion about method vs property, but I like signaling to user that there is zero overhead to this call (it doesn't need to be cached).

[grgbkr]

Started looking into this.

Currently Replicache implements ReadTransaction, and Replicache has a clientID property of type Promise<string>.

export class Replicache<MD extends MutatorDefs = {}>
  implements ReadTransaction
{
  ...
  get clientID(): Promise<string> {
    return this._clientIDPromise;
  }
  ...
}

For Replicache this does need to return a Promise, as the clientID is not known when Replicache is constructed, as it has to be loaded from indexDB.

Currently <Read|Write>TransactionsImpl are all created by Replicache, and in an async manner which would easily allow us to await the client ID promise and construct them with a concrete clientID. It's really only Replicache itself being a ReadTransaction that creates a difficulty.

Options

1. We simply make the interface return a Promise. I don't currently understand the use cases for transactions using this clientID and how onerous it is for it to be a Promise. As a user of this API, I might suspect this is more expensive to retrieve than it really is.

interface ReadTransaction {
+ readonly clientID: Promise<string>;
}

2. Involves changing existing API Update Replicache to no longer implement ReadTransaction, and add readonly clientID: string to ReadTransaction. I don't know the history of having Replicache implement ReadTransaction. I believe it is a convenience that allows you to simplify code a tiny bit by not calling Replicache#query. I see the nicety of it, however, its also confusing as multiple calls to Replicache#<get|has|isEmpty|scan> are not in fact transactional (each creates a new transaction). If Replicache no longer implements ReadTransaction we can easily add a non-Promise clientID to ReadTransaction.

3. Involves changing existing API We get rid of the public constructor of Replicache and instead have an async factory function which returns a Promise<Replicache>. When this promise resolves the Replicache instance will already be "open", that is the DB and client id will have been initialized. Then we can use a readonly clientID: string for both ReadTransaction and Replicache.

[aboodman]

Ah, good observation.

Separately I filed an RFE about Replicache.clientID being asynchronous (#542).

It was thought that this feature enhancement here partly subsumes #542, but it wouldn't if we make ReadTransaction.clientID async. So we shouldn't do that.

There is also an option 4: to have query() and subscribe() return a new interface: ReadTransactionWithClientID, then have WriteTransaction inherit that.

... or alternately option 5: extract out a separate interface, maybe called Read that is exactly the currently contents of ReadTransaction. Then have ReadTransaction inherit Read but add clientID.

Of these I suppose I like your option 2 best. I'm not sure there's any value to having Replicache implement ReadTransaction and I agree with your critique that it's confusing. Second best I like option 5. Both of these are breaking changes, but they are minor and I don't think anyone is relying on the fact that Replicache implements ReadTransaction. We would still bump the major version anyway, though.

[arv]

A fourth alternative is to have query etc take a ReadTransaction & ClientIDRead

I think I'm leaning towards 1 or 2. Having Replicache implement ReadTransaction is not very useful IMO.

[aboodman]

Wait actually @arv - master of the versioning - would making Replicache no longer implement ReadTransaction be a breaking change, if it still has all the methods that were previously on ReadTransaction.

This gets super deep into: are our typescript types part of our interface? I guess so yes. Is whether you implement a type (by name) part of your interface? I think I could construct situations where changing this would be detectible by users and break them, but it would be difficult!

[arv]

You are right. This is not a breaking change if the interface that has clientID is not called ReadTransaction. We can do:

interface ReadTransactionNewName extends ReadTransaction {
  readonly clientID: string;
}

[aboodman]

No my question is: if we do option (2) is that a breaking change? What about my option (4)?

Breakingness doesn't affect what I think we should do here, I'm just curious.

PS @grgbkr I also want to do your option 3, but it's complicated. See #542.

[grgbkr]

Thanks for the input.

Regarding Aaron's ReadTransactionWithClientID and Arv's ReadTransaction & ClientIDRead option, I also considered this but think it makes the type hierarchy overly convoluted.

I thought about Aaron's option 5 Read and ReadTransaction. I like this a bit better since we get rid of Replicache being a ReadTransaction and have it implement Read instead. However, its pretty arbitrary that clientID is in ReadTransaction and not Read, if we did #542 we'd probably want to push clientID up to Read. Then we have a sub-interface that adds nothing but the semantic marker that it is transactional. Not the worst, but a bit convoluted.

I'm pretty convinced Repliache should not implement ReadTransaction. You would not want to pass Replicache to an arbitrary function that takes a ReadTransaction, since it is not transactional and thus runs foul of the Liskov Substitution Principle.

I'm going to put together a PR for my option 2 (Replicache no longer implements ReadTransaction), as I think its the best way forward. I'll also look a bit more at what needs doing for #542.

[arv]

Option 2 FTW

[aboodman]

Great argument. 100% convinced, thanks!

[grgbkr]

This had to be reverted due to an unexpected perf regression in populate and createIndex performance tests
277f6ff#commitcomment-58522932

Reverts:
4da3f52
976bf98

Will investigate underlying cause and attempt to fix and re-land.