search

ottypes / docs

277 stars 29 forks source link

Add presence-related properties #27

Open gkubisa opened 6 years ago

gkubisa commented 6 years ago

Adds 3 new optional properties, which are necessary for synchronizing presence using ShareDB.

curran commented 5 years ago

Is the isOwnOp argument really necessary in transformPresence(presence, op, isOwnOp)?

Related discussion in https://github.com/ottypes/json1/issues/9

Also, my gut feeling is that comparePresence could be replaced by a standard deepEqual, without the need to add anything to the OT spec.

The createPresence function really serves to validate the passed in presence object, nothing more as far as I can tell. Perhaps this also could be removed from the spec, and application code could opt into calling some type-specific validation method.

gkubisa commented 5 years ago

createPresence is analogous to create and turns some initial data into a valid presence object, which might be different from the initial data. I added createPresence mostly for consistency and would actually question the need for both functions.

I agree that comparePresence should not be needed.

Re isOwnOp, transformPresence might be used in contexts where transforming against own op is what's needed. IMHO, keeping that param makes the API complete, as it allows the client code to control conflict resolution in a way similar to the side param in transform.

curran commented 5 years ago

From what I've seen in trying to use the presence API in ShareDB, application code is expected to pass in an already-valid presence object to createPresence. In what case would something other than a valid presence object be passed into createPresence? The only case I can imagine is when an application developer makes a mistake, in which case createPresence serves mainly to validate. Perhaps the createPresence method could be replaced by an optional validatePresence or isValidPresence function. This could even be outside the scope of the spec, as it's pretty much for developer ergonomics during development, and not essential to the operation of presence.

Regarding isOwnOp, you may be correct that there are contexts where transforming against own op is what's needed, but I'm having trouble imagining one. Would you be able to provide a concrete example of this? It appears that whenever you emit an op yourself, it also makes sense to emit new presence data, if for nothing other than incrementing the c count (so the app can blink things to indicate the user is active). But, maybe that's not valid to assume (one may not want to emit presence on every op, if they don't need to support anything like a blinking feature).

gkubisa commented 5 years ago

I completely agree with what you said about createPresence and validation. I'd be happy to get rid of createPresence. Feel free to update the presence feature in ShareDB accordingly.

But, maybe that's not valid to assume (one may not want to emit presence on every op, if they don't need to support anything like a blinking feature).

Exactly. Additionally, if I remember well, in the current presence implementation in ShareDB the presence is published only when there are no ops pending. So, if a user submits ops fast, the presence could become significantly delayed. Thanks to transforming against own ops, the other clients can immediately update the presence locally without waiting for presence messages.