WIP Presence #286
WIP Presence #286
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.
1 similar comment
|
The failing test in CI is |
|
I don't get the advantage of |
|
If anything, I would change those to I'm not sure about the performance of |
|
Thanks @gkubisa for the clarification. Since these objects are often generated externally, and may need to survive The only further change I'm considering is to make the implementation robust to OT types that do not implement The following could serve as defaults if these methods are not provided by OT types (from this json0 fork): json.createPresence = function(presenceData) {
return presenceData;
};
json.comparePresence = function(pres1, pres2) {
return JSON.stringify(pres1) === JSON.stringify(pres2);
};This could just as well be left as future work too. Thoughts? |
|
I moved presence logic into a new module, but this is only really used for It could look something like this: var Presence = require('./presence');
// in various different files...
Object.assign(Doc.prototype, Presence.docMethods);
Object.assign(Agent.prototype, Presence.agentMethods);
Object.assign(Connection.prototype, Presence.connectionMethods);
Object.assign(Backend.prototype, Presence.backendMethods);I think I'll stop for now and await review feedback. Kindly requesting review by @nateps @ericyhwang @gkubisa . Thanks in advance! |
There was a problem hiding this comment.
Notes from the PR review call with @nateps @ericyhwang @alecgibson :
- Name
presence.currentcould be better. - Maybe presence should be its own class? Now it's a bunch of functions that refer to
this.presence. Maybe it should be more like a Presence class with a Document reference. Decorating with methods is sort of a code smell. The Document could create a Presence instance. It would be a lot more clear.this.doc. ...would be more explicit. - Doc is the biggest place to focus. Agent, Connection, Backend are not a big concern.
- Instead of
enablePresence, we could take something else, perhaps code. This could be similar to pubSub, where different implementations could be passed in. The implementations might be radically different. There could be a "dummy" default one that doesn't do anything. Options.presencewould be on the backend. That should have some effect on all connections usingbackend.createConnection. Might also be required to configure the Connection (client) as well.- Passing something in that implements an interface could reduce bundle size as well, if it's not included.
- On the server we could either do what we do with PubSub - default implementation is simple, could pass in an alternative. Default could be "presence is not enabled", so app code could import the basic implementation.
- That Presence class should be passed to the Connection constructor.
- A separate Presence class should be passed into the Backend constructor.
- These could live in a presence directory, sharedb/lib/presence - Server impl of presence. Pass this to Backend.
- Also a client/presence.js - this can be passed to the Connection constructor.
- Generally presence should be more loosely coupled.
- Instance on the connection vs. doc.
- Would be nice to be able to support the use case of multiple presences (be at multiple places within a document). This is possible now to do outside shareDB, in OT type.
- Would also be nice to let application code specify the
transformPresencefunction. - Could even be so decoupled where you could subscribe only to presence.
- From Connection, could pass collection and doc ID.
- Ideally Presence object/interface would not be coupled to a doc at all.
In summary (immediately actionable):
- presence logic should not decorate Doc with methods, and should have minimal interactions/references with Doc internals.
- Instead of a boolean flag, the presence class instance should be passed into Connection and Backend constructors.
|
Stuck on this failing test: Skipping that test, the next failing test may reveal the issue: Stopping for now. |
|
Closing in favor of #288 |
A continuation of #207.
While resolving merge conflicts, I encountered the following:
4024 - OT Type does not support presence- I changed this to4027, as the code4024was introduced to mean something else since this PR was submitted.Doc.prototype._hardRollback- This PR introduces a solution for not dropping errors, whereas a competing solution already existed (from [Update hard rollback callback logic](Update hard rollback callback logic) by @alecgibson ). I did my best to reconcile the two, favoring the one already presence in master.