Add Promise-based versions of methods in public ShareDB API

[ericyhwang]

The last time this was brought up was back in 2018, in #202, @nateps expressed said he wasn't really a fan on promises in ShareDB. However, I can't find his reasoning written down anywhere.

Nate's not really an active maintainer anymore ever since handing ShareDB off to @alecgibson and me a while back, so the two of us can decide to add promises if we want :)

I know Alec's a fan of promises in general, so let's try and get it done!

After thinking through it and doing some testing, I think we should:

• Add new promise versions of the async methods, instead of turning the existing ones into hybrid callback-or-promise.
  • The hybrid approach interacts poorly with the Connection and Doc .on('error', listener) "default error handler" listeners
  • See below for details. I suspect this issue with the hybrid approach was partly what Nate had concerns about.
• Have the new methods immediately throw if a Promise implementation isn't globally available.
  • No shimming of Promise inside ShareDB. If a consumer wants to use promises and still needs to support IE 11, they can install a global Promise polyfill at the root of their app, if they don't already have one.

Thoughts, questions, comments, concerns? Anything I missed in my analysis below?

Adding new promisified versions of the methods

Example, using TypeScript syntax:

class Doc {
  // Existing callback-based method remains unchanged
  submitOp(op: Op, options: Options, callback?: Callback): void;
  submitOp(op: Op, callback?: Callback): void;
  // New Promise-based method
  submitOpPromised(op: Op, options?: Options): Promise<void>;
}

Advantages:

• This is entirely backwards-compatible. If a consumer still wants to call submitOp() a bunch of times synchronously and then use Doc#on('error', listener) to handle errors, they can still do so.
• If a consumer calls the new submitOpPromised, that's an explicit signal that they intend to use the returned promise, including correctly handling errors, so we don't have to worry about emitting it to the Doc error listener.
• Simpler method signatures

Downside, we have double the number of such methods, and consumers have to change to calling the new method. However, even with the next auto-promise version, consumers would still have to add then/catch or await to their call sites to actually take advantage of the promises, so I feel like it's not a big deal to have them call a separate method.

Automatically returning Promise if no callback is given

Advantages:

• Many hybrid callback/promise APIs take this approach, and [WIP] Promisify client-facing methods #225 is an example of how this might work in ShareDB.
• No need to call a new method, though consumers would still need to add either then/catch or await at the call sites to actually use promises, so they'd be modifying those lines in their code anyways.
• Fewer methods in the ShareDB API, though each method has a more complicated call signature.

Problem 1

As that PR description alludes to, this would be a breaking change for on('error', listener), which acts as a default "unhandled error" handler at the Connection/Doc level when an explicit callback isn't provided to a method that accepts one. We could make this non-breaking with some work, see further down.

There are tests exercising that the error event is emitted when no callback is given, such as this one:

sharedb/test/client/submit.js

Lines 875 to 883 in 083f8ac

	doc.create({age: 3});
	expect(doc.version).equal(null);
	expect(doc.data).eql({age: 3});
	doc.on('error', function(err) {
	expect(err.message).equal('Custom error');
	expect(doc.version).equal(0);
	expect(doc.data).equal(undefined);
	done();
	});

If we start auto-returning a Promise:

• Say that doc.create({age: 3}); starts returning a Promise since no callback was given.
• The promise gets rejected, and because there's no handling of the rejection, the global unhandled promise rejection handling gets triggered.
• In Node 15+, that defaults to terminating the Node process. That's despite the presence of the doc.on('error', ...) handler.

There is a way to make this backwards-compatible with more work. In the callback-to-promise shim code, when calling reject(errorFromCallback), also check if connectionOrDoc.listenerCount('error') > 0. If so, then add a no-op catch to the original promise to not trigger the global unhandled exception handler, and emit the error on the Connection/Doc.

Problem 2

With the backwards-compatible workaround, the Connection/Doc error listener will fire even for errors that get handled further on in a promise chain or higher up in the stack. It no longer solely acts as a Connection/Doc-level handler for otherwise unhandled errors, since it receives errors that did get handled.

Couple examples, where the error handler receives an error that gets handled:

doc.on('error', console.log);
function() {
  doc.create({age: 3})
    .then(processResult)
    .catch(handleError);
}
async () => {
  try {
    return doc.create({age: 3});
  } catch (error) {
    handleError(error);
  }
}

There's not really a clean way around this, since the handling of the error isn't directly on the original promise. A process/window level unhandled rejection listener that forwards back to the Connection/Doc could mostly work. One issue, the listener for sharedb wouldn't be able to prevent the event from continuing on to other unhandled rejection listeners.

[alecgibson]

I personally find submitOpPromised() kind of icky, but it would solve the backwards-compatibility issues.

On the other hand, I think I wouldn't mind just breaking the API to get Promises.

As far as the 'error' event goes, I haven't thought about it too hard, but one alternative is to just remove the 'error' event completely (from the client): using Promises already represents a paradigm shift, where rejections must be handled (or else the process terminates). Handling your rejections is best practice (which is why Node.js enforces this behaviour), and would basically replace the need for the 'error' event in the first place. Obviously this would be breaking, but I think it would be worth it to reduce the API footprint, as well as confusion around error handling (we'd otherwise have three ways of handling errors: in callbacks, promise rejections, and events!).

[curran]

I'd favor the breaking change. That approach seems overall simpler in that it places fewer constraints on the ShareDB internals. Less complexity, fewer tests required, lower chance of bugs.

I have written functions that wrap the ShareDB API in Promises so many times. It would be amazing if ShareDB supported Promises out of the box!

That said, I wonder how much work it would be to upgrade Racer and Derby to use the new API. Then the onus would be on those packages provide backwards compatibility or not, for all the products built on them.

[ericyhwang]

If we did eventually want to make a breaking change, we can and should put the new behavior behind a feature flag first, to give interested consumers time to prep their code in advance, and provide some migration recommendations.

The "error" event handlers are the big sticking point as far as backwards compatibility goes. That kind of error handling is not super unusual for Node APIs, especially for libraries that can be used in a more "fire-and-forget" manner, like using Redis for logging or event queueing, where you do synchronous-looking calls in application code and have the library handle errors centrally. ShareDB could be used like that, though I don't know how common that is.

Difficulty of migration depends on how many fire-and-forget calls to fetch, subscribe, submitOp, etc. there are in your codebase. Tens of calls in one package is pretty easy. Hundreds of calls spread around different packages, where a single Backend gets passed into different packages... that's much harder, even with a feature flag. We could look into either implementing or recommending an unhandled rejection shim to help.

On my end, updating Racer would not be difficult with either approach, since Racer doesn't directly expose the ShareDB API. Racer's calls into the ShareDB Doc are contained into a single Racer RemoteDoc class, so Racer can just always pass a callback for ShareDB calls and then do its own thing for error dispatch.

Which is to say, either approach for promises, it's not much work to migrate "my" usages of ShareDB, but in general I do want to make sure we're considering difficulty of migration when talking about breaking changes.

As far as API footprint goes:

I will point out that either approach adds the same amount of API footprint, it's just whether or not the new API footprint is bundled into a method overload or broken out into a separate method. Writing out formal TS signatures makes that clear:

// New method, existing method signatures unchanged
class Doc {
  submitOp(op: Op, options: Options, callback?: Callback): void;
  submitOp(op: Op, callback?: Callback): void;
  submitOpPromised(op: Op, options?: Options): Promise<void>;
}
// New overload for existing method, existing signatures slightly change
class Doc {
  // Callback becomes required in existing void-return signatures
  submitOp(op: Op, options: Options, callback: Callback): void;
  submitOp(op: Op, callback: Callback): void;
  // New signature for overload with no callback and returned Promise
  submitOp(op: Op, options?: Options): Promise<void>;
}

You could argue that, fully expanding out the optional params, it's actually 5 total signatures for new-method vs 4 signatures for new-overload. The extra signature in the new-method approach is the existing submitOp(op: Op): void; signature, which goes away in the new-overload approach since that'd return a Promise.

[alecgibson]

I think I'm personally leaning towards the hybrid callback/Promise API. It's certainly not uncommon, and it's a bit "nicer" for consumers. I'm not all that concerned with the API footprint, just with the ickiness of the method name 😛 If we do this, then I think we should drop emitting error events, since the unhandled promise rejection fulfills the same role as the unhandled error event, and I can't really see a reason to have both.

I agree that we should hide this behind a feature flag, which would just toggle you between "callbacks & error emission" vs "callbacks & promises".

The main difficulty for people migrating is if you're listening out for a "generic" error event on eg your Doc to catch all of your unhandled callback errors in a fire-and-forget fashion. I guess the best thing to do here is make sure we have a nice hierarchy of errors, eg a base ShareDBError class which all of our errors use, and then maybe a per-class subclass eg ShareDBDocError. Then if people want to replicate existing doc.on('error') behaviour, they can use something like:

window.addEventListener('unhandledrejection', (event) => {
  if (event.reason instanceof ShareDBDocError) {
    // Move doc.on('error') code here
  }
});

[ericyhwang]

I was thinking of attaching the Doc or Connection to the Error, that way your global handler could check for the error.shareDbDoc or error.shareDbConnection.

In the past, I've had instanceof checks not work properly in global code while going through the process of dependency major version updates in private repos, since instanceof doesn't work across two different installed versions in the package tree.

---

Thoughts on whether the flag to enable promises should be "global" or specific to each Connection?

[alecgibson]

Attaching non-serialisable objects to an Error always makes me feel a little uncomfortable, since the first thing most people do is try to just log the thing to console / send up to an alert service, etc.

If instanceof makes you nervous, we could just use a good old-fashioned property:

if (error.isShareDBError) {
  switch (error.code) {
  }
}

Thoughts on whether the flag to enable promises should be "global" or specific to each Connection?

I can't immediately think of a good case where — in the same codebase — you'd want different code styles/error handling on a per-connection basis. I think we can just force people all-in. If they have some (weird) reason that this doesn't work, they can stay opted-out, and raise a use case with us?

[curran]

Promises would be super nice. I'm currently wrapping the API like this:

// Fetches a ShareDB Doc.
// See https://share.github.io/sharedb/api/doc
const fetchDoc = (
  collectionName: string,
  id: string,
  shareDBConnection: ShareDBConnection
): ShareDBDoc =>
  new Promise((resolve, reject) => {
    const shareDBDoc = shareDBConnection.get(collectionName, id);
    shareDBDoc.fetch((error) => {
      error ? reject(error) : resolve(shareDBDoc);
    });
  });

// Saves a fetched ShareDB doc (upsert).
const saveDoc = (doc: ShareDBDoc, data) =>
  new Promise((resolve, reject) => {
    const callback = (error) => (error ? reject(error) : resolve(null));
    if (!doc.type) {
      doc.create(data, callback);
    } else {
      doc.submitOp(diff(doc.data, data), callback);
    }
  });

// Deletes a fetched ShareDB doc.
const deleteDoc = (doc: ShareDBDoc, data) =>
  new Promise((resolve, reject) => {
    doc.del((error) => (error ? reject(error) : resolve(null)));
  });

Invocations look like this:

await saveDoc(await fetchShareDBDoc(COLLECTION, id, shareDBConnection), data);
...
await deleteDoc(await fetchShareDBDoc(COLLECTION, id, shareDBConnection));

[alecgibson]

When we (eventually) get around to doing this, it might also be nice to add support for the proposd Explicit Resource Management? TypeScript just added support for it in v5.2, and it would be super nice to write things like:

using connection = backend.connect(...);
await using doc = connection.get(...);
// do stuff and don't worry about having to call doc.destroy() or conneciton.close() etc.

Assuming we've switched to Promises, I think it just involves aliasing things like:

Doc.prototype[Symbol.asyncDispose] = Doc.prototype.destroy;
Connection.prototype[Symbol.dispose] = Connection.prototype.close;

[alecgibson]

Revisited today:

• Let's go with a global flag to opt in to this behaviour
• If opted-in, then we use a hybrid callback/promise API to ease migration
• Any methods without a callback provided will return a Promise which may reject (instead of emitting 'error')
• Any methods with a callback do not return a Promise to avoid undefined behaviour