Replicate 'Delete doc with rev in opts'

[AGBrown]

Summary

The api docs imply two (and only two) overloads for remove,

1. remove(docId: string, docRev: string, options?: { ...
2. remove(doc: { _rev: string; _id: string; }, options?: { ...

_{(where the doc in the overload (2) is currently typed to ExistingDoc)}

The test 'Delete doc with rev in opts' (see below) implies a third overload:

1. remove(doc: NewDoc, options: { rev: string }, ...

Question

Is this (overload 3 above) an official overload and do we want therefore want it in the d.ts?

Samples

1. non-compiling MCVE: gist and playground (note red squiggle on line 34)
2. compiling MCVE: gist diff showing changes and playground

If playground url doesn't work, paste the gists into http://www.typescriptlang.org/Playground

Details

.js test from test.basics.js (L386):

it('Delete doc with rev in opts', function () {
  var db = new PouchDB(dbs.name);
  var doc = {_id: 'foo'};
  return db.put(doc).then(function (info) {
    return db.remove(doc, {rev: info.rev});
  });
});

.ts equivalent:

it('Delete doc with rev in opts', () => {
  var db = new PouchDB(dbs.name);
  // type specified purely to illustrate this question
  var doc: pouchdb.api.methods.NewDoc = { _id: 'foo' };
  return db.put(doc).then((info) => {
    return db.remove(doc, { rev: info.rev });
  });
});

As of 2c043b96 (current feat branch at the time of writing this) the above .ts snippet would not compile as

1. doc, being a NewDoc ({_id: string;}) does not have a _rev: string; property - that is only present in ExistingDoc
2. this is due to developing the remove overloads according to the api doc for remove() which shows only two valid function definitions and states (highlighting added):

doc is required to be a document with at least an _id and a _rev property.

Based on this documentation these are the only two overloads defined to date for remove() in pouchdb v3.4.0.

[marten-de-vries]

The documentation often doesn't contain all the allowed forms, just to keep the whole thing understandable. Supplying a rev in the options might have been added for backwards compatibility or some syntactic sugar or some other reason, but since it's not explicitly deprecated I think in this case you can consider it supported. (Mostly because it is tested.)

On the other hand, it's not a widely used function signature as far as I can tell (I'm pretty sure I don't have logic handling it in pouchdb-wrappers, for example. But that's more a bug from my side than PouchDB's.)

[AGBrown]

IRC chat log:

[20:45:47] AndyBrown: I don't think that options object is used for anything. but it is documented

[20:45:56] http://pouchdb.com/api.html#delete_document

[20:47:03] well, there is this rev option AndyBrown talked about that's tested. And I pretty much add options to anything using pouchdb-wrappers to complicate matters. :P

[20:47:28] commandoline: nah, I don't think it complicates

[20:47:40] I like that our API consistently takes an options object right before the callback, just in case

[20:49:08] nolanlawson: There are a couple of places where the code doesn't allow that (I should know, I needed to code exceptions for all these functions for Python-PouchDB), but it could be added trivially.

[20:50:46] AndyBrown: ... A realistic example of it would be pouchdb-security, which wants a {userCtx: {...}, secObj: {...}} object. ... [20:54:37] <AndyBrown> nolanlawson, commandoline: so I'm ok with the options objects - they are in the d.ts overloads I am building. Types on function parameters in typescript specify the minimum required shape - they are there primarily to (a) ensure the user supplies at least enough information, not to restrict what other properties can be on an object and (b) to help the user supply the right stuff through intellisense.

[20:54:37] <AndyBrown> So I'm probably just looking for you guys to say that either you want an overload which lets the user specify the rev in the options, or if you want to stick to just the two overloads explicitly detailed in the docs. Keep in mind this d.ts will reflect your intentions in v3.4.0 and we can change it in future.

[21:00:02] AndyBrown: well, for 3.4.0 you can be pretty sure it's going to be a supported overload because of that test you reference. I can't really imagine a scenario where the user needs it

[21:00:09] <AndyBrown> and in fact I think that's probably the actual question: do you (who are more qualified than I to decide) want the 3rd overload for the v3.4.0 d.ts, or not.

[21:00:22] <AndyBrown> commandoline: decision made then [I'll include it if possible]

[21:00:30] But since it's just an option, and the 3rd one at that, it probably doesn't hurt to add it.

[AGBrown]

Decision: we will add this, but it is low priority for the moment as it is not publicly documented, it is just in one of the tests, and the current overload definitions do fulfil the api docs as explicitly stated. Assigning it as up-for-grabs for anyone to fork and PR.