how should functions that return promises be handled in JSDoc?

[jbphet]

I'm working on some code that uses native JavaScript promises and async/await, and I'm not sure how to document the return types in JSDoc. Since this is new for PhET, this discussion may well set the standard for how this is done going forward.

Looking around online, it seems like there isn't a general consensus on this (see jsdoc/jsdoc#509 and, specifically, jsdoc/jsdoc#509 (comment)). The problem is that a Promise resolves to some value or throws an error, which makes it quite different from most return types. I'm leaning towards something like this:

@returns {Promise<string>}

...but in a recent meeting where this was discussed, @samreid suggested this:

@returns {Promise<string, Error>}

Someone else - I can't recall whether it was @mattpen or @zepumph - suggesting using @throws too.

Part II of this question is how to document async functions. There is a pretty clear example here http://usejsdoc.org/tags-async.html, though the @async and @function tags seem redundant to me and are unlikely to be picked up by PhET. So, if we remove them, the example would look like this:

/**
 * Download data from the specified URL.
 *
 * @param {string} url - The URL to download from.
 * @return {Promise<string>} The data from the URL.
 */

I'd like to get input from @jonathanolson and @mattpen first, since they are also writing async code using the latest JS support, and then we can open it up to the team.

[samreid]

Where can I see context for this (a specific case where you are facing this problem)? I searched Download data from the specified URL. across master and didn't find anything.

[jonathanolson]

Including the type of the resolved value sounds great. Not sure about the utility of reject types. I’m switching to only using Error subtypes, sometimes including custom subtypes (see execute.js in perennial master). This would be nice to optionally document, but specifying “Error” everywhere sounds like something I’d want to avoid.

[zepumph]

@jonathanolson I agree. In the last dev meeting @mattpen seemed to be on the same page as you. If there is a specific Error subtype that is worth documenting/is custom, then we though that we could use @throws to dictate that. Something like:

/**
 * Have a party and invite all of your friends
 *
 * @param {Date} date - The date of the party.
 * @return {Promise<Boolean>} Whether or not the party is on.
 * @throws {HasNoFriendsForPartyError} - If there are no friends to invite to the party.
 */

I'm not sure if the @throws type would be the Error subtype itself, or a Promise of type Error subtype. The alternative marking would be:
@throws {Promise.<HasNoFriendsForPartyError>}

Either seem reasonable to me because it leaves the documentation of the reject/error case up to the developer.

[jbphet]

@samreid - for a specific example, please see LongTermStringStorage.getStrings.

[samreid]

Thanks for the context, I don't have anything to add beyond what @jonathanolson and @zepumph and @jbphet said.

[mattpen]

I think @throws {Promise.<HasNoFriendsForPartyError>} is preferable over @throws {HasNoFriendsForPartyError}. The former instance indicates that the error can be thrown asynchronously with Promise.reject, whereas the latter looks like it only throws synchronous errors

[pixelzoom]

In 12/14/17 developer meeting, we decided 2 things (1) create this issue, (2) defer decision until @jonathanolson is available. So labeling for dev meeting.

[pixelzoom]

Most of the above examples (#169 (comment), #169 (comment)) are using incorrect JSdoc syntax. E.g. {Promise<string>} should be (Promise.<string>} -- note the missing '.'

[jonathanolson]

I think @throws {Promise.} is preferable over @throws {HasNoFriendsForPartyError}. The former instance indicates that the error can be thrown asynchronously with Promise.reject, whereas the latter looks like it only throws synchronous errors

So use of @throws is interesting. Note that if you throw an error in an async function, it rejects the promise with that error (instead of having the throw propagate normally). It definitely doesn't throw a Promise. If we're going to use that to document promise rejection types, I'd prefer @throws {HasNoFriendsForPartyError}.

[jonathanolson]

For example, perennial's execute.js would have the documentation:

/**
 * Executes a command, with specific arguments and in a specific directory (cwd).
 * @public
 *
 * Promise will be rejected if the exit code is non-zero.
 *
 * @param {string} cmd - The process to execute. Should be on the current path.
 * @param {Array.<string>} args - Array of arguments. No need to extra-quote things.
 * @param {string} cwd - The working directory where the process should be run from
 * @returns {Promise.<string>} - Resolves with a string of the full stdout from the process.
 * @throws {ExecuteError} - Includes information about the stdout and exit code
 */

[mattpen]

Perhaps we should reserve @throws for syncronous exceptions and use @reject or @rejects for rejected promises, as in jsdoc/jsdoc#509?

[pixelzoom]

@jbphet is out this week, and he started this issue. So does this need to be deferred to a future dev meeting?

[pixelzoom]

12/21/17 dev meeting:

Go with what @jonathanolson proposed in #169 (comment) for now.

Assigned to @jbphet to see if he has any objections. If so, please re-label for discussion at a future developer meeting.

[pixelzoom]

@ariel-phet I removed the "deferred" label so that @jbphet reviews this when he returns.

[jbphet]

I would prefer @Rejects instead of @throws (as Matt suggested in #169 (comment)), since it is always a promise that is returned and exceptions are not truly thrown. So, I'd like to modify the proposal to (this is a specific example but showing generally the style):

/**
 * Executes a command, with specific arguments and in a specific directory (cwd).
 * @public
 *
 * Promise will be rejected if the exit code is non-zero.
 *
 * @param {string} cmd - The process to execute. Should be on the current path.
 * @param {Array.<string>} args - Array of arguments. No need to extra-quote things.
 * @param {string} cwd - The working directory where the process should be run from
 * @returns {Promise.<string>} - Resolves with a string of the full stdout from the process.
 * @rejects {ExecuteError} - Includes information about the stdout and exit code
 */

[jonathanolson]

The above proposal in #169 (comment) sounds great to me.

[pixelzoom]

I have no experience with promises, so I'll give #169 (comment) a +0.5.

[samreid]

Sounds good, you have my upvote.

[ariel-phet]

@jonathanolson will be updating relevant code with this style of documentation

[jonathanolson]

Also to note, the standard rejection would be of type {Error}, but since that would be included in every async function, it should be omitted. Only include the @rejects if it is for a non-standard error type.

[jonathanolson]

Applied the documentation changes, closing.