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

Adding presence to ot-json0 #25

Closed
wants to merge 6 commits into from
Closed

Conversation

houshuang
Copy link

No description provided.

Copy link

@curran curran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool!

The package name changed here.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "ot-json0",
"version": "1.1.0",
"name": "@houshuang/ot-json0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems out of place for this PR.

lib/json0.js Outdated
// of the same subtype, not sure if this is a given.
// We're only concerned about the first level of object/array,
// not sure if the spec allows nesting of subtypes.
json.transformPresence = function(presence, op, isOwn) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thrilled to see this development!

It would be awesome if these functions were covered by tests.

Copy link

@curran curran Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next step would be to handle transformation of nested structures recursively.

Also, it would be interesting to implement presence for text0, since it's embedded within json0.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be interesting to implement presence for text0, since it's embedded within json0.

@houshuang
Copy link
Author

In fact, that PR was submitted in error - I was trying to submit against a different upstream :) I thought I closed it, but I guess I didn't. Anyway we are absolutely happy to share this work, just not quite done yet (but feedback welcome!). Here's a quick demo video with Quill and presence/shared cursors. https://www.youtube.com/watch?v=CVjH56Q18cU

I'll probably not handle nested subtypes right now, but would love to see someone tackle that. I will however work on text0, and also a very simple approach to registering presence in a field (with no transformations). I guess it makes no sense to submit PR to this repo unless they merge the main presence PR to sharedb core... Unfortunately ot-json0 is hard-coded into sharedb, which I want to try to change, because we would prefer to use @Teamwork fork (or main sharedb if presence gets merged), however right now that's impossible with a different ot-json0 implementation. (It complains "Wrong basic type" or something like that).

Also want to add tests. Need to see what already exists, and what I can add.

@curran
Copy link

curran commented Apr 9, 2019

@houshuang Thanks for your response!

It is indeed unfortunate that ot-json0 is hard-coded into ShareDB. My gut feeling is that ShareDB should be able to handle a different type as the base type for documents, but I've never tried it.

I'm leaning towards using the @Teamwork fork as well, mainly for its presence implementation.

I'm thrilled to hear you're interested in working on text0. I will be following this work closely and perhaps we can collaborate on it, as I'm keen to implement presence text0 strings that are deeply nested within json0 documents.

I stumbled on this, which may be interesting: Interesting Cursor Transform Work by Robert Lord #27

@houshuang
Copy link
Author

houshuang commented Apr 9, 2019 via email

@curran
Copy link

curran commented Apr 10, 2019

I started continuing this work on presence over here datavis-tech#2

@curran
Copy link

curran commented Apr 10, 2019

@houshuang That error might make a good issue on the ShareDB repo. My gut feeling is that, in principle, you should be able to set the default OT type.

@houshuang
Copy link
Author

Superseded by #31

@houshuang houshuang closed this Apr 26, 2019
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