Support @promise annotation

[crandmck]

To document a Promise-based API, strong-docs will need to support the @promise annotation.
See strongloop/loopback#418.

[bajtos]

strong-docs will need to support the @promise annotation.

@ritch I'm not sure if this is needed. What would the annotation do?

[crandmck]

@ritch should respond with his thoughts, but I assumed that for a function, this annotation would display an additional signature (in addition to the standard asyc/callback signature) to clearly indicate (a) the function does support a promise API and (b) what the promise signature is, e.g. the expected argument to then() and catch().

While this might be obvious to someone used to promises, it will be helpful for the many folks who are not, and will also be more explicit than just saying "this function supports promise API". In any case, we need to make it really easy for someone referring to the API docs to understand how to call a given function both with callbacks and with promises.

[ritch]

@ritch I'm not sure if this is needed. What would the annotation do?

It should make it obvious how to use the Promise style of the API / method. Right now it is implied that we support promises... this @promise annotation would make that explicit.

[chandadharap]

Reassigning to Yappa for immediate attention.

[hacksparrow]

On it.

[hacksparrow]

@crandmck @ritch @chandadharap @bajtos any thoughts on the "syntax" and display template for this annotation?

[crandmck]

Everyone's out on holiday until at least 1/4, and I'm out starting later today until 1/7. Meantime, refer to #63 (comment) above.

Although I'm not terribly familiar with promises, I assume we want to do something along these lines. You'll want to confirm with @ritch or @bajtos before proceeding, but this might help you get started until we hear from them.

Consider PersistedModel.find(). Here's the current doc comment:

/**
   * Find all model instances that match `filter` specification.
   * See [Querying models](http://docs.strongloop.com/display/LB/Querying+models).
   *
   * @options {Object} [filter] Optional Filter JSON object; see below.
   * @property {String|Object|Array} fields Identify fields to include in return result.
   * <br/>See [Fields filter](http://docs.strongloop.com/display/LB/Fields+filter).
   * ...
   *
   * @callback {Function} callback Callback function called with `(err, returned-instances)` arguments.    Required.
   * @param {Error} err Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).
   * @param {Array} models Model instances matching the filter, or null if none found.
   */

This results in https://apidocs.strongloop.com/loopback/#persistedmodel-find.

For promises, I assume we'd want something like this:

/**
   * Find all model instances that match `filter` specification
   * using promise API.
   *
   * @promise
   *
   * @options {Object} [filter] Optional Filter JSON object; see below.
   * @property {String|Object|Array} fields Identify fields to include in return result.
   * <br/>See [Fields filter](http://docs.strongloop.com/display/LB/Fields+filter).
   * ...
   *
   * @promise-result {Function} result Function called with result if operation succeeds.
   * @promise-err {Function} err Function called with err object if there is an error.
   */

It's unclear to me if the success function always takes an object arg, or if it can be anything, and if the error function gets an Error object or something else. So, again, you'll need to talk with Ritchie and/or Miroslav.

[hacksparrow]

Thanks @crandmck, that should be enough for me to get started, we can settle on the details later.

[hacksparrow]

So this is the syntax I have come up with:

* @promise
* @resolve {Array} Model instances matching the filter, or null if none found.
* @reject {Error} Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).

which yields this:

Functionality implemented, but not pushed yet (need to clean up a bit). Any suggestions?

@ritch @bajtos @crandmck @chandadharap

[bajtos]

@hacksparrow your proposal is a good start, I am wondering if we can simplify it a bit.

I think @reject {Error} is redundant. While it's certainly possible to reject a promise with a non-error value, the best practice is to always use Error instances to signal errors (AFAIK). Can we make @reject annotation optional, and let strong-remoting fill in default info?

Now that most functions will need only @promise and @resolve annotations, I am proposing to implement a short-hand syntax:

 * @promise {Array} Model instances matching the filter, or null if none found.

---

Another aspect to consider is duplicate information. At the moment, we would end up with something like this:

 * @callback {Function} callback Callback function called with `(err, returned-instances)` arguments.    Required.
 * @param {Error} err Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).
 * @param {Array} models Model instances matching the filter, or null if none found.

 * @promise
 * @resolve {Array} Model instances matching the filter, or null if none found.

Can we come up with a syntax that does not require us to describe the result twice, once for the callback and second time for the promise?

---

The screenshot of proposed API doc looks good. I'd like to add a bit of text to explain what the Promise section documents and how to actually get a promise back. For example,

**Callback** (optional)

Optional callback. When the callback function is not provided, 
a promise is returned instead (see below).

[table with callback args]

**Promise**

This method supports both callback-based and promise-based invocation.
Call this method with no callback argument to get back a promise instead.

[table with resolve and reject]

---

@ritch @raymondfeng @hacksparrow thoughts?

[hacksparrow]

@bajtos excellent suggestions, it is very much in sync with what I'd prefer. Will make the suggested changes. Thanks!

[hacksparrow]

@ritch why is sdocs using iolo/dox instead of the one published on npm?

[hacksparrow]

Implemented on branch feature/promise.

@crandmck scroll down on the README for the details on how to use it.

@bajtos @ritch please review.

I had to update one aspect of an existing test to make it compatible with the use of the official dox module: Error= changed to Error|undefined and Object= changed to Object|undefined.

[crandmck]

@hacksparrow I've got it working on my laptop now, but I'm seeing one problem.

I added the @promise annotation to PersistedModel.replicate since this comment says that it supports Promise invocation.

It displays properly, however, in the console I see this:

Promise cannot be resolved in PersistedModel.replicate()
Description for resolve object not found in PersistedModel.replicate()

Not sure if that is significant or not.

[jannyHou]

@crandmck @hacksparrow
I think PersistedModel.replicate gives warning due to this line:
* @param {Conflict[]} conflicts A list of changes that could not be replicated due to conflicts.

seems promise doesn't read the syntax of Array from callback.

[hacksparrow]

@crandmck @jannyHou there is a formatting error at https://github.com/strongloop/loopback/blob/master/lib/persisted-model.js#L928

It should be

@param {Object} checkpoints The new checkpoints to use as the "since"

not

@param {Object] checkpoints The new checkpoints to use as the "since"

The return parameter count is more than two, therefore this method cannot be converted to a promise, hence the description for resolve object will not be found.

[jannyHou]

thank you @hacksparrow I am triaging one issue about similar problem, and yes I think we should put return parameters in an array to make promise work, like findOrCreate

[bajtos]

I believe this has been implemented and landed by #67, I am closing this issue as done. Please open a new one if there is any more work left.