Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add presence-related properties #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gkubisa
Copy link
Contributor

@gkubisa gkubisa commented Apr 26, 2018

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

This was referenced Apr 10, 2019
@curran
Copy link
Contributor

curran commented Apr 13, 2019

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

Related discussion in ottypes/json1#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.

@curran curran mentioned this pull request Apr 15, 2019
@gkubisa
Copy link
Contributor Author

gkubisa commented Apr 15, 2019

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
Copy link
Contributor

curran commented Apr 15, 2019

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
Copy link
Contributor Author

gkubisa commented Apr 15, 2019

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.

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.

2 participants