modernize code

[dcharbonnier]

It would be nice to modernize this code, this include :

• using es6 syntax (classes, exports, let, const)
• using promises and only promises
• using typescript or at least write a type definition
• dropping support for callback, browserchannel, etc.
• expect.js is a dead project
• add linter / formater
• callback with real Error objects, remove things like callback({ code: 4008, message: 'Unknown type' })

[curran]

Seems like a heavy lift without much gain to me. Also it would break working code and require a major version release (and the project is not even at 1.0 yet).

As a user of ShareDB not using TypeScript, what do I stand to gain from the proposed changes?

[curran]

This would make sense to do in a hard fork, then release 1.0 there.

I fear this project will never ever reach 1.0. /cc @nateps

[dcharbonnier]

The only point that would require a change for the users of the library is
"- dropping support for callback, browserchannel, etc."
I'm not personally favorable on hacking a code to support both callback and promises but it's something possible.
The fact that the library will never reach 1.0 is not so important IMO, better have a 2.0 with a code that look like 2018 than an already outdated 1.0.
The benefits of TS is to be able to refactor the code with less pain, to generate a proper documentation, to avoid some of the tests, to provide interfaces for the development of plugins or database drivers (Today this is a reverse engineering work which make me quite uncomfortable).
You will find plenty of blog posts of projects moving from JS to TS and the benefits, if you find one against it please share.

[curran]

Indeed, the benefit for users of ShareDB seems to be the switch from callbacks to Promises.

The other benefits are for maintainers and contributors.

[dcharbonnier]

Example, it you want to create a new PubSub you just need to extend the abstract PubSub :

export interface IPubSubOptions {
    prefix?: string;
}
export interface IStream {
    id: string;
}
export declare abstract class PubSub {
    private static shallowCopy(object);
    protected prefix?: string;
    protected nextStreamId: number;
    protected streamsCount: number;
    protected streams: {
        [channel: string]: IStream;
    };
    protected subscribed: {
        [channel: string]: boolean;
    };
    protected constructor(options?: IPubSubOptions);
    close(callback?: (err:Error|null) => void): void;
    publish(channels: string[], data: object, callback: (err: Error | null) => void): void;
    subscribe(channel: string, callback: (err: Error | null, stream?: IStream) => void): void;
    protected abstract _subscribe(channel: string, callback: (err: Error | null) => void): void;
    protected abstract _unsubscribe(channel: string, callback: (err: Error | null) => void): void;
    protected abstract _publish(channels: string[], data: Object, callback: (err: Error | null) => void): void;
    protected _emit(channel: string, data: object): void;
    private _createStream(channel);
    private _removeStream(channel, stream);
}

This definition is not manually generated, it's just the existing code with types (and a modernization even if I didn't changed the callback to Promise because I wanted exactly the same implementation)
https://github.com/dcharbonnier/sharedb/blob/master/src/lib/pubsub/pubsub.ts

[dcharbonnier]

we could probably make more stuff private but because there is no contract in the current library implementation I think only @nateps knows.

[alecgibson]

As much as I would love to see the death of all callbacks in this codebase, I agree that it might be a lot of work.

As a stop-gap, would it be possible to at least return Promises to consumers of the library?

At the moment, my own codebase is littered with code like:

function submitOp(document, op) {
  return Promise((resolve, reject) => {
    document.submitOp(op, (error) => {
      if (error) {
        reject(error);
      } else {
        resolve();
      }
    });
  });
}

whereas obviously I'd love to be able to just write:

async function() {
  // Do stuff
  await document.submitOp(op);
  // Do other stuff
}

We could definitely add this support without dropping callback support. It's a bit icky, but it's definitely a common pattern when trying to support both styles.

If people are keen for this, I could try to go around and replace method calls by wrapping callbacks:

Document.prototype.fetch = function(..., callback) {
  // Do stuff...

  return new Promise((resolve, reject) => {
    var wrappedCallback = function (error) {
      callback(error);
      if (error) {
        reject(error);
      } else {
        resolve();
      }
    };

    doThingThatPassesOnCallback(..., wrappedCallback);
  });
};

That way you could support both Promises and callbacks, and offer consumers a much nicer API.

[alecgibson]

So I got kind of curious about this, and started a quick spike into a proof-of-concept switch over to TypeScript. I ported all the files in lib/client to TypeScript. The tests (practically untouched) all pass. I haven't bothered putting together any sort of build chain, so it doesn't actually currently output a usable module, but that wouldn't be hard.

https://github.com/share/sharedb/compare/master...alecgibson:typescript?expand=1

A couple of my thoughts from this exercise:

• This can be done without breaking external APIs (including even the require mechanism)
• ts-lint + VS Code is a great combo
• I tried removing all _underscoreMethods, and in doing so realised that actually a lot of external modules often refer to them. So I take it an underscore is meant to mark a method that is public, but only for use by other ShareDb modules? I personally always read them as shorthand for private. Feels a bit like a code smell. Would need sorting out if we exposed this API.
• I didn't try implementing Promises, but it looks doable, especially if we create a utility wrapper method for wrapping a callback. Alternatively, we could potentially write a TypeScript decorator like @promisify() (I'm personally a big fan of the AOP that decorators enable, but they're an experimental feature, so on balance unfortunately probably not right for a library like ShareDb)
• Tests could do with upgrading at some point, but I suspect the entire codebase should be refactored into TypeScript with the old tests passing, and then as a separate step the tests could be refactored to use Mocha/Sinon, or whatever people prefer.

What do people think? I'm personally a huge fan of TypeScript. I think it's a spectacular tool for refactoring with confidence (on top of tests of course!), and some of the modules in here really could do with some refactoring (the Doc class is almost 1000 lines long, for example).

Finishing the job would still be a chunky bit of work (probably another week or so of full-time work on top of what I've already done). I'm not sure I really have time for it, but could try to keep chipping away at it, if people think it's worthwhile? Alternatively, we could set up a build pipeline on what I've done so far and keep incrementally updating the codebase, given that it retains backwards-compatibility?

[dcharbonnier]

I agree obviously, low effort compared to the benefits. The goal is still that this original project get back on track anyway and not to multiplicate the forks. I would be able to contribute actively on the initial effort and on maintaining this but let's see if @gkubisa who has the most interesting fork would agree on that. I have 2 small pending PR on teamwork and they don't get any comments or merge so that may also be a dead end.

[alecgibson]

Yes. It's a shame about all the forking that happens (we may have to work on a fork for document history if my other PR doesn't get merged). But if we do want to add TypeScript, I can definitely try to help chip away at this.

[curran]

I'd love to see the move to TypeScript.

Also yes, it would be great to use Promises in the public API, even if the internals are left mostly untouched.

Also I've been toying with the idea of exposing ShareDB subscriptions as Observables, which seems a more natural representation than the current callback system, or Promises. But I digress, that would be a separate issue.

[gkubisa]

TypeScript and Promises in the public API sound great to me. I'll certainly accept the PRs in @teamwork/sharedb. I will also go through all the other PRs this week (I didn't do it yet because I was on holidays).

Hopefully we'll have a PR review on Wednesday and get the main project going again, see #163 (comment). If not, I'll continue supporting the forks at @Teamwork.

[alecgibson]

@gkubisa I've had a very quick spike into returning Promises on Doc here: #225

[gkubisa]

During the meeting last week @nateps was not keen to the idea of Promises in ShareDB. :-( Re the @teamwork/sharedb* forks, if there's no chance of introducing Promises to the official repos, than I'd rather stay away from them in my forks too, to avoid maintenance overhead.

[alecgibson]

Ah I missed that. That's a shame. Guess I'll close that then (but for the record, I really, really, really hate callbacks).

[soney]

FYI, I made a preliminary type definition file for ShareDB in DefinitelyTyped. The PR is here: DefinitelyTyped/DefinitelyTyped#28089

I also created a TypeScript wrapper for ShareDB here: https://github.com/soney/sdb-ts

There's still lots of work (and I'm not sure that the style will match everyone's preferred TS style) but I have found it helpful in a few of my own projects.

[kmannislands]

Would also like to see this happen. A lot of the code would be much easier to grok as TS/modern js of some sort.

As for the promise/callback thing, it's pretty trivial to support both with ~30 lines of vanillas js, so might as well, right? I know I promise-wrapped ShareDB for my usages, imagine many others have too.

Willing to contribute to see it happen but a little confused about the various forks and how they relate.

[ericyhwang]

We discussed Typescript briefly at the weekly PR discussion meeting, and we all do think it's a good idea. (I somehow missed this thread when I started doing some ShareDB work.)

Looks like there are two major ideas here. My current thoughts are that it'd be better to separate them out, to keep discussion focused on each and so it's easier to make progress.

1. Typescript - This should be doable without changing the API, which is nice.
   • Adding *.d.ts type definitions is a relatively straightforward step, which doesn't necessarily require source code updates. Looks like @soney already added an initial version - thanks! 🎉
   • An in-place conversion to *.ts might take a bit longer and could make it harder for forks to sync from upstream. Though it looks like @alecgibson already started and didn't find it to be too bad.
2. Promise-ify - Technically would be adding to the API, at the very least.
   • Did @nateps have a particular concern with promises? (I can ask next week.)
   • The hybrid approach via wrapper looks interesting, but I don't have much experience with the usability of hybrid callback-or-promise libraries. Anyone who does have thoughts?
   • Would Typescript / type defs play well with the hybrid approach?

---

Willing to contribute to see it happen but a little confused about the various forks and how they relate.

This is the original repo, published as sharedb on NPM. There wasn't much active maintenance work for a while, up until about 6 months ago, so there are understandably some longer-running forks. @nateps, the main maintainer here, is working on getting additional maintainers onboarded - at the moment, me and @alecgibson.

Right now, the largest fork I'm aware of is the Teamwork fork maintained by @gkubisa , which has user presence broadcasting. I believe the Teamwork fork pulls in upstream changes occasionally, so non-breaking changes here should also show up there.

From both ends, we do want to get presence into the original repo here. No ETA yet, though, as it's a big feature, and finding enough time has been tricky.

[alecgibson]

I'm happy to contribute some time to this (although I don't know how much I can promise). I hopefully have a better understanding of the library as a whole now, so should be able to make a better stab at it than last time.

You're right - we can definitely do these separately. In theory, we can do TypeScript class by class (because it's a superset of JS), although in practice I tend to find that the first pass winds up requiring defining interfaces for everything anyway.

I'm keen for Promises. Pretty sure hybrid Promises/callbacks are fine in TypeScript. I think you can overload methods in TypeScript.

I guess the thing to do is get a priority call on one or the other of these. I personally would find both super beneficial and a huge help. In theory both can be done in non-breaking ways, so I don't think there's any priority defined by that.

[dcharbonnier]

I can not dedicate time for that but if you have questions feel free to contact me.
Moving to typescript won't take too much time. My strategy is to start with a lot of any so you get the general structure ; then you start removing the any. You should have no any at the end.
The most complicated and time consuming task is when you find a "bug" in the code base (and you will, you always do), you have to understand it to fix it.
Happy to see that happening, you open the door to more good quality contributions :-)

[gkubisa]

I'm happy to see ShareDB moving to TypeScript and Promises. Unfortunately, I won't have time to help with that.

Regarding the Teamwork fork, I've been merging the latest upstream changes occasionally as they integrated cleanly, however, I'm not likely to have time to resolve bigger conflicts. To be honest, I'm rather reluctant to support the Teamwork fork in the long term. All my changes (presence and undo/redo in particular) are in the PRs, so feel free to tweak them as you see fit and merge (or just close the PRs :-) ), ideally before starting any work on TypeScript and Promises.

[kikonejacob]

@dcharbonnier @gkubisa any update on this in 2020? Should this issue be reopened?

[issacclee]

it seems like the @types/sharedb is still targeting the an outdated version of sharedb, is the types package built automatically from source? or do we have to manually update it?

[alecgibson]

@issacclee it's manual. I've raised DefinitelyTyped/DefinitelyTyped#61202

[issacclee]

@issacclee it's manual. I've raised DefinitelyTyped/DefinitelyTyped#61202

Thanks for your work. It seems like the current type definition still has some inconsistency compare to the documentation. For example the Backend Api doc states that it contains a method called "submit" . However there's no such definition in the index.d.ts file.

[alecgibson]

It seems like the current type definition still has some inconsistency compare to the documentation.

Yes, this is because the types were written manually. Please feel free to raise a Pull Request!