-
Notifications
You must be signed in to change notification settings - Fork 453
Presence 2 #288
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
Presence 2 #288
Conversation
I had to add the --exit flag workaround to mocha.opts to make it exit when tests are done. A better long-term solution would be to ensure that nothing keeps node running after all tests are done, see https://boneskull.com/mocha-v4-nears-release/#mochawontforceexit.
The problem was that unsubscribe re-added the doc to the connection. Now the doc is removed from the connection after unsubscribe. Additionally, we're no longer waiting for the unsubscribe response before executing the callback. It is consistent with Query, unsubscribe can't fail anyway and the subscribed state is updated synchronously on the client side.
The issue could not cause problems in practice because ot-json0 does not support presence.
@@ -91,6 +108,8 @@ __Options__ | |||
* `options.pubsub` _(instance of `ShareDB.PubSub`)_ | |||
Notify other ShareDB processes when data changes | |||
through this pub/sub adapter. Defaults to `ShareDB.MemoryPubSub()`. | |||
* `options.presence` _(implementation of presence classes)_ | |||
Enable user presence synchronization. The value of `options.presence` option is expected to contain implementations of the classes `DocPresence`, `ConnectionPresence`, `AgentPresence`, and `BackendPresence`. Logic related to presence is encapsulated within these classes, so it is possible develop additional third party presence implementations external to ShareDB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this API. It feels atypical both of normal JS, and also of this library. I think I'd rather pass in a single presence
object which exposes API methods, much like the DB adapters.
@@ -308,6 +327,9 @@ Unique document ID | |||
`doc.data` _(Object)_ | |||
Document contents. Available after document is fetched or subscribed to. | |||
|
|||
`doc.presence` _(Object)_ | |||
Each property under `doc.presence` contains presence data shared by a client subscribed to this document. The property name is an empty string for this client's data and connection IDs for other clients' data. The structure of the presence object is defined by the OT type of the document (for example, in [ot-rich-text](https://github.com/Teamwork/ot-rich-text#presence) and [@datavis-tech/json0](https://github.com/datavis-tech/json0#presence)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree with handing this off to the types, because:
- I'm not sure it's strictly part of "OT", and it feels like a violation of Single Responsibility
- This relies on us being able to update the type libraries (can we?)
- There are potentially multiple desired implementations for a given type. Especially if you consider - for example -
json0
, which can sub-type other arbitrary types. What is a "generic" shape for this presence?
To me, it feels more natural for us to define some sort of separate presence transformers, probably being passed in to the presence
constructor option?
@@ -467,6 +497,10 @@ Additional fields may be added to the error object for debugging context dependi | |||
* 4022 - Database adapter does not support queries | |||
* 4023 - Cannot project snapshots of this type | |||
* 4024 - Invalid version | |||
* 4025 - Passing options to subscribe has not been implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(NB: We'll need to change these to string error codes: #309)
|
||
// Expose the DocPresence passed in through the constructor | ||
// to the Doc class, which has access to the connection. | ||
connection.DocPresence = this.presence.DocPresence; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only work when the client and the server are on the same box. It's not going to work for the mainline case, where the client resides on a remote machine, and we can't directly access Doc
from Backend
.
// to the Doc class, which has access to the connection. | ||
connection.DocPresence = this.presence.DocPresence; | ||
|
||
connection._connectionPresence = new this.presence.ConnectionPresence(connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I stated above, I don't think this looks "normal". I'd possibly expect something more along the lines of:
presence = this.presence.connectionPresence(connection);
@@ -255,6 +253,9 @@ Connection.prototype.handleMessage = function(message) { | |||
return; | |||
|
|||
default: | |||
if (this._connectionPresence.isPresenceMessage(message)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like leaning on default
for this - why can't we give it its own message type?
@@ -58,6 +68,8 @@ function Doc(connection, collection, id) { | |||
this.type = null; | |||
this.data = undefined; | |||
|
|||
this._docPresence = new connection.DocPresence(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I've mentioned above, Doc
won't have access to connection.DocPresence
the way this is currently put together, because connection.DocPresence
is set by Backend
, which might be on a different machine altogether.
@@ -48,6 +55,10 @@ function Backend(options) { | |||
if (!options.disableSpaceDelimitedActions) { | |||
this._shimAfterSubmit(); | |||
} | |||
|
|||
this.presence = options.presence || dummyPresence; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for having this off by default? Performance?
@@ -0,0 +1,6 @@ | |||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this file?
// The processed presence is removed to avoid leaking memory, in case peers keep connecting and disconnecting a lot. | ||
// The processed presence is not removed immediately to enable avoiding race conditions, where messages with lower | ||
// sequence number arrive after messages with higher sequence numbers. | ||
this.receivedTimeout = 60000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get some justification about why a minute is a good default amount of time? (ie is it because it's a long time compared to the "standard" lifecycle of a submit request?). How does this work if we're on a patchy connection?
Also nit: can we please write 60 * 1000
?
Okay, I've tried to actually use this, but it doesn't currently work if your client isn't on the same box as your server (as outlined inline). I've also had a quick skim through the code, and here are my personal thoughts: I like that this attempts to stay as generic as possible, but I worry that it's gone too far. I actually think that I think I've said in the past that I'd expected presence to act more like its own entity, and that you should be able to just do things like In the future, if we wish to add pluggable behaviour, I think we should expose presence middleware hooks. For example, if you wanted to keep a stateful cache of presence on the server, then perhaps there would be a An alternative to middleware might be that we have a PresenceCacheDB, which again by default no-ops, but can be replaced by an instance that actually serves cached presence. But - in my opinion - this is all stuff that can be added after implementing the basics (this was very much how we approached document history and milestones). I'm also not a fan of delegating presence functionality to types. I don't think it should be their responsibility, and I think there might be many "shapes" for a presence object for a given type, depending on application. I personally think we should configure presence transformers in the new Backend({
presence: {
cacheDb: new MemoryPresenceCacheDB(),
transformers: {
'rich-text': (presence, delta, isOwnOp) => { ... },
},
},
}); |
All excellent ideas and feedback @alecgibson ! /cc @gkubisa , the original author of this work. All I did with this PR was to try to rearrange his work such that the PR could be accepted. I do love the idea that presence should be a different responsibility than the OT types. It was a pain to fork JSON0 and add presence to it. |
Okay, I might try to have a stab at this today with a blank piece of paper, but drawing heavily on what we've done here. |
That would be amazing! Happy to review your work. If you haven't seen it, the presence feature in https://github.com/SyncOT/SyncOT might be interesting. I do not understand it, just have seen there's work done. Presence is implemented there as its own package under https://github.com/SyncOT/SyncOT/tree/master/packages/presence/src |
Just to give a bit of background, SyncOT is my take on OT with somewhat different trade-offs in some key areas. As I could design it from the ground up, I came up with a bit different way of handling presence, which you might want to adapt to ShareDB. Note that SyncOT is not complete and the ideas presented below have not been tested in real-life situations, apart from Global Presence which we're currently using in production as a stand-alone component separate from OT. Back to presence in the context of OT... the main idea is to decouple Global Presence and Local Presence: Global Presence represents user sessions/connections. It contains 3 mandatory properties: Local Presence represents users' presence within OT documents, eg caret positions, etc. It is stored as part of OT documents, updated through operations and transformed as needed. It is associated with Global Presence using From the above you could notice that Global Presence is transient and Local Presence is persistent, which leads to some problems:
I hope the above makes sense and might help implement presence in ShareDB. |
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
FWIW I'm in favor of the work over in #322 . It looks a lot cleaner than this implementation at first glance. |
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
Closing this in favor of #322 |
This change adds the ability for clients to broadcast information about "Presence" - the notion of a client's position or state in a particular document. This might be represent a cursor in a text document, or a highlighted field in a more complex JSON document, or any other transient, current information about a client that shouldn't necessarily be stored in the document's chain of ops. The main complication that this feature solves is the issue of keeping presence correctly associated with the version of a `Doc` it was created at. For example, in a "naive" implementation of presence, presence information can arrive ahead of or behind ops, which - in a text-based example - can cause the cursor to "jitter" around the change. Using the ShareDB implementation will ensure that the presence is correctly transformed against any ops, and will ensure that presence information is always consistent with the version of the document. We also locally transform existing presence, which should help to keep (static) remote presence correctly positioned, independent of latency. In order to facilitate this, the feature must be used with an OT type that supports presence. The only requirement for supporting presence is the support of a `transformPresence` method: ```javascript type.transformPresence(presence, op, isOwnOperation): presence; ``` * `presence` _Object_: the presence data being transformed. The type will define this shape to be whatever is appropriate for the type. * `op` _Op_: the operation against which to transform the presence * `isOwnOperation`: _boolean_: whether the presence and the op have the same "owner". This information can be useful for some types to break ties when transforming a presence, for example as used in [`rich-text`][1] This work is based on the [work][2] by @gkubisa and @curran, but with the following aims: - avoid modifying the existing `Doc` class as much as possible, and instead use lifecycle hooks - keep presence separate as its own conceptual entity - break the presence subscriptions apart from `Doc` subscriptions (although in practice, the two are obviously tightly coupled) - allow multiple presences on a single `Doc` on the same `Connection` [1]: https://github.com/quilljs/delta#tranformposition [2]: #288
Summary of changes
statelessPresence
, which can be used like this:presence
option is expected to contain implementations of the following classes:DocPresence
, which is instantiated atdoc._docPresence
.ConnectionPresence
, which is instantiated atdoc._connectionPresence
.AgentPresence
, which is instantiated atdoc._agentPresence
.BackendPresence
, which is instantiated atdoc._backendPresence
.Doc
,Connection
,Agent
andBackend
invoke methods on their respective presence classes.dummyPresence
is used, which implements methods that mostly no-ops.statelessPresence
, sodoc.presence
, as in the original implementation.