Skip to content

A specific version of ot-json0 is hardcoded as the basic type #284

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

Open
houshuang opened this issue Apr 10, 2019 · 10 comments
Open

A specific version of ot-json0 is hardcoded as the basic type #284

houshuang opened this issue Apr 10, 2019 · 10 comments

Comments

@houshuang
Copy link

There's currently a bunch of experiments going on around adding presence to ot-json0. One of the things I've come across is that ShareDB currently has a hard dependency on the ot-json0 NPM package, meaning that if we want to replace it with another implementation, we need to also fork the ShareDB package, which is unfortunate.

Here's the code that errors out:

      if (types.map[message.type] !== types.defaultType) {
        err = new ShareDBError(4020, 'Invalid default type');
        return this.emit('error', err);

(lib/client/connection.js:198)

We can either remove that (I did, and it worked), or instead of this line in lib/types.js:

exports.defaultType = require('ot-json0').type;

we might think about letting the user (optionally) supply a default type when configuring ShareDB?

Currently I've just been looking at replacing the base ot-json0 implementation with a type that is almost identical, just with added presence. However, I don't actually know enough about the requirements for a defaultType - does there have to be one? Does it just need to "exist" or to implement a certain interface (beyond the general OT interface)? Could I specify "ot-text" as a default type?

Thanks!

@alecgibson
Copy link
Collaborator

We're running on a forked version of json0, but the master release of sharedb. This is our workaround:

const sharedb = require('sharedb');
const sharedbClient = require('sharedb/lib/client');
const richText = require('rich-text');
const json0 = require('forked-ot-json0');

registerTypes(sharedb);
registerTypes(sharedbClient);

function registerTypes(library) {
  library.types.register(json0.type);
  // ShareDB starts with a default type of JSON0, from its own dependency
  // (ie not our forked version). We need to override ShareDB's default with 
  // our forked JSON0, otherwise we get an "Invalid default type" error on
  // connection.
  library.types.defaultType = json0.type;
}

export const Backend = sharedb;
export const Client = sharedbClient;

However, yes, I wholly agree that there should be an official way of overriding this type.

@curran
Copy link
Contributor

curran commented Apr 10, 2019

Woohoo! The power of Open Source. I knew someone must have faced this at some point.

@ericyhwang
Copy link
Contributor

Comments from @nateps in weekly meeting:

  • The server and client do need to agree about the default type, which the snippet does do.
  • He originally wanted to bake in an easy way to make this configurable, but then got caught up in looking at how it would interact with bundlers like browserify and didn't get around to it.
  • Getting server and client to agree could be done in the init handshake, perhaps.
  • Perhaps use a default type per connection, that's set up when initiating the connection. During handshake, client sends its default type, and server stores that on the agent.
  • That would allow you to version the types too.

Further thoughts:

  • @alecgibson - One concern is that if someone forks json0 and doesn't change the URI, then two types with the same URL could be incompatible.
  • Nate - One solution would be to put a semver in the type URI, like via a query parameter.

@curran
Copy link
Contributor

curran commented Apr 11, 2019

This may be a bit extreme, but what if ShareDB did not depend on json0? It could just be a devDependency used only in tests. The code that uses ShareDB could pass in the type as an argument to the constructor.

Put another way, if application code uses ShareDB but doesn't use json0, then why should it be in their bundle? That's 9.1 kB minified for no good reason.

I recently discovered json1 and text-unicode, which look to be very solid and fresh OT types from @josephg. These may be the best types to use for longer-term development of ShareDB-based applications.

@curran
Copy link
Contributor

curran commented Apr 15, 2019

Currently it looks like this, in both server and client:

const json0 = require('fork-of-ot-json0');
const ShareDB = require('sharedb'); // or require('sharedb/lib/client');
ShareDB.types.register(json0.type);
ShareDB.types.defaultType = json0.type;

That's not too bad, but what if we had a cleaner API, and made a breaking change that requires application code set the default type, not providing json0 as a built-in default? That would add a bit of boilerplate, but I think decoupling ShareDB from json0 would be a big enough win to merit it. It could look like this:

const json0 = require('fork-of-ot-json0');
const ShareDB = require('sharedb'); // or require('sharedb/lib/client');
ShareDB.setDefaultType(json0.type);

This could be behind a deprecation flag pre-1.0. Thoughts on this setDefaultType proposal?

@alecgibson
Copy link
Collaborator

I think I'm a fan of the approach where clients negotiate with the server in the connection handshake to set a default type on a per-connection basis. This seems more sensible to me than just setting it across all connections.

@curran
Copy link
Contributor

curran commented Apr 22, 2019

@alecgibson Why would it be preferable to set a default type on a per-connection basis (negotiate in the connection handshake), as opposed to setting it globally for the app (all connections)? I'm having trouble imagining a scenario where that would be preferable. Do you have one in mind?

@alecgibson
Copy link
Collaborator

I personally think that setting a default type conceptually belongs on the connection, in the same way that a client / server will negotiate eg TLS version / cypher on a per-connection basis. It's some configuration that should be left to the client to decide, because - after all - they'll be the one actually manipulating the type.

These are some of the possible trade-offs:

Negotiate on handshake

  • allows different clients to use different default types (eg when certain clients only care about particular collections; don't forget that even when you're connecting to ShareDB on a "server" machine, you're actually still acting as a client)
  • is a bit more robust to client/server not having the same default type. For example, let's say we manage to add semver to JSON0 (a whole other issue). Because of caching, etc., the client may have an older version of the JSON0 type than the server. In this case, the server can still be shimmed to support the older version, and they can negotiate this in their handshake, avoiding breaking clients by bumping the JSON0 type on the server
  • defining the default type on connection is a bit more explicit, and makes sure that the client knows exactly which default type they're dealing with

Server dictates default type

  • simpler to implement
  • simpler to consume

@curran
Copy link
Contributor

curran commented May 1, 2019

If you're starting a fresh project, this is not an issue, but I think it makes sense for migrations/rollouts of larger deployments.

@curran
Copy link
Contributor

curran commented May 29, 2019

Note: the handshake is useful/necessary for app deployments that upgrade the OT type.

At least an error should be thrown if the client and server protocols (OT types) don't match.

In a sense, there will be no more "default type", and instead we'll have a type defined per connection.

Extra step for mismatched clients: convert op from e.g. json0 to json1 before applying it.

JingYenLoh added a commit to Orgbital/orgbital-server that referenced this issue Jun 3, 2020
The json0 appears to be a hard dependency of sharedb (c.f.
share/sharedb#284). To handle plain text
rather than JSON documents, it appears that we have to register the
'type' with sharedb.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants