Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Ability to document callback arguments #260

Closed
cowwoc opened this Issue · 29 comments

9 participants

@cowwoc

We need a way to document callback argument. For example, given:

var MyFunction = function(onSuccess, onFailure) { ... }

I need to document the arguments I will be passing into onSuccess and what return values are acceptable. There are workarounds for documenting the callback parameters, but they result in documentation that is hard to read. There is no known workaround for documenting the callback return values.

@hegemonic
Owner

I completely agree that this is needed. I'll confer with @micmath; there have been many different syntax proposals for this in the past, and I'm not familiar with most of them.

@cowwoc

My 2 cents...

This is a recursive problem. The callback could have its own callback. As I don't think we should document callbacks in-line. One approach would be to uniquely name each callback and @link to its documentation. We should be able to document the callback parameter briefly and link the user to a more detailed explanation. For example: "this callback is invoked on success. See {@link MyCallback} for more information".

@qubyte

+1 I could really use this right now! :)

@hegemonic
Owner

Via email, @micmath pointed out that there's already a way to do this by documenting the callback as an inner function. No new tags required! Here's an example:

/**
 * Classy class.
 * @class
 */
function TestClass() {}

/**
 * Called after doing asynchronous stuff.
 * @name TestClass~TestCb
 * @function
 * @param {String} err - Information about the error.
 * @param {Number} int - An integer of joy.
 * @return undefined
 */

/**
 * Do asynchronous stuff.
 * @param {string} name - A name.
 * @param {TestClass~TestCb} callback - The callback function.
 * @return undefined
 */
TestClass.prototype.doAsyncStuff = function(name, callback) {
    // ...
};

I believe @micmath is planning to say more about this on the mailing list.

Leaving this ticket open for now as a reminder that we really need to document this approach.

@micmath
Owner

That plan is underway. I'll put something together this weekend.

@micmath
Owner

Simplest possible example:

/**
 * @class
 */
function MyClass() {}


/**
 * Do asynchronous stuff.
 * @param {MyClass~onSuccess} onSuccess - Called if doing asynchronous stuff succeeds.
 */
MyClass.prototype.doAsyncStuff = function(onSuccess) {
    // ...
};

/**
 * @function MyClass~onSuccess
 * @param {String} result - The result of the asynchronous process.
 * @param {Number} int - A result code.
 * @return undefined
 */

If you needed to document things about callback parameters immediately you can follow that pattern. It might be possible to sugar this sort of thing up in a slightly sweeter syntax by patching JSDoc.

@neekey

@micmath Thanks for providing such kind of way to document callback params, just wonder if you are still working on that slightly sweeter syntax.

@micmath
Owner
@cowwoc

Michael,

I highly dislike this syntax. It involves cramming the description of an entire callback and its parameters into a long line. What happens if a callback takes another callback as an argument? What then? This approach does not scale.

@hegemonic
Owner

I have to say that the callback syntax that's already supported (as described above in @micmath's MyClass~onSuccess example) seems pretty darned good to me. It's far more straightforward than the many other suggestions I've seen on the mailing list in the past. It also scales well for nested callbacks.

I agree with @cowwoc that the Closure Compiler syntax does not lend itself to clear documentation. Fortunately, we already support something much better!

Again, I'm just keeping this issue open as a reminder that we need to document the existing syntax and share it with the mailing list.

@micmath
Owner

@cowwoc I highly agree with you. It's not a nice syntax. In fact it's awful.

@neekey

@micmath It seems like there is still some problem with the way you provided:

The code below will only generate a simple title for Next without description of params.

/**
 * An simple tool set.
 * @type {Object}
 */
var Tool = {};

/**
 * Do something Async...
 * @param {Next} next
 */
Tool.getURL = function( next ){

};

/**
 * Called if Fetching URL is done.
 * @function Next
 * @param {String} url
 * @returns undefined
 */

And if I add ~ like below:

/**
 * Called if Fetching URL is done.
 * @function Tool~Next
 * @param {String} url
 * @returns undefined
 */

Than the Next will become the inner method for Tool, but still, the link from the description of Tool.getURL will be an anchor like Tool#Next which is incorrect.

@neekey

@micmath I found an issue from YUIDoc, with tag @callback to indicate a method as callback, yui/yuidoc#33

@neekey

or in JSDuck they just use @param to document callback senchalabs/jsduck#57 , like below:

statics: {
        /**
         * A method documented as static.
         * @param {Function} fn Callback function.
         * @param {Ext.Component} fn.cmp The component parameter.
         * @param {Number} fn.index Component index parameter.
         * @param {Boolean} fn.return The return value of callback
         * must be true to include the component, false to exclude.
         * @param {Object} scope Scope for the callback.
         * @return {Ext.Component[]} Array of components.
         */
        filter: function(fn, scope) {
            return this.items.filter(fn, scope);
        }
    }

It seems like that this approach is much more simple and can be commonly used.

@cowwoc

The JSDuck approach almost as bad as Google's Closure Compiler: it's an ugly hack that does not scale for nested callbacks.

The end-goal should be readability, not ease of implementation.

@neekey

@cowwoc hmm.. get your point, but as so far, the JSDuck way seems like the only way that can work for the common situation, event for nested callbacks, although not so scaleable.

Maybe @callback is a better, Whether readability or for nested callback:

/**
 * An simple tool set.
 * @type {Object}
 */
var Tool = {};

/**
 * Do something Async...
 * @param {AsyncCallback} next
 */
Tool.getURL = function( next ){};

/**
 * Called if Fetching URL is done.
 * @callback AsyncCallback
 * @param {String} url
 * @param {fetchCallback} fcb 
 * @returns undefined
 */

/**
 * the callback for callback `Next`
 * @callback fetchCallback
 * @param ....
 */
@cowwoc

+1 for neekey's proposal :) Though I believe you have a typo (@returns should read @return)

@ejzn

+1 For this feature for sure.

@micmath
Owner

@neekey,

Thanks for your thoughts on this, a concrete, specific proposal always wins over a general feature request. The first issue I see with this is that under your proposal callbacks are not guaranteed to have unique namepaths. JSDoc uses namepaths to resolve things like @link tags. The architecture of JSDoc guarantees that each documented symbol will have a fully unique string that refers only to that symbol. As an example, this is perfectly allowed in JavaScript:

var boo = true;

function someFunc(boo) {
}

How would we document those 3 things, the var, the function and the callback? Following your example we get:

/** @var boo */
var boo = true;

/**
  @function someFunc
  @param {boo} boo
 */
function someFunc(boo) {
}

/**
  @callback boo
 */

But the problem is that now have 2 things with the namepath "boo", one is a var and the other a callback. The problem gets worse in a more realistic application where you could have multiple functions all with different callbacks that have the same name.

/**
  @function someFunc
  @param {???} cb - A callback that takes a boolean.
 */
function someFunc(cb) {
}

/**
  @function someOtherFunc
  @param {???} cb - A callback that takes a string and a number.
 */
function someOtherFunc(cb) {
}

/**
  @function yetAnotherFunc
  @param {???} cb - A callback that takes two objects and an array.
 */
function yetAnotherFunc(cb) {
}

That's not even unlikely source code in the wild. So we need a way to distinguish one cb from another. Your example implies we could invent unique names and refer to them, like so:

/**
  @function someFunc
  @param {cbCallback1} cb - A callback that takes a boolean.
 */
function someFunc(cb) {
}

/**
  @function someOtherFunc
  @param {cbCallback2} cb - A callback that takes a string and a number.
 */
function someOtherFunc(cb) {
}

/**
  @function yetAnotherFunc
  @param {cbCallback3} cb - A callback that takes two objects and an array.
 */
function yetAnotherFunc(cb) {
}

But that seems a little arbitrary to me, why choose the pattern ___Callback(n)? That makes name collisions less likely but technically you could still have a var named "cbCallback3" defined somewhere else in your application (no matter how unlikely). Why not use a naming convention that is systematic and guaranteed to always be unique? Turns out there is one already available, just prefix the name of your callback with the name of the owner function and a "~" character, like so:

/**
  @function someFunc
  @param {someFunc~cb} cb - A callback that takes a boolean.
 */
function someFunc(cb) {
}

/**
  @function someOtherFunc
  @param {someOtherFunc~cb} cb - A callback that takes a string and a number.
 */
function someOtherFunc(cb) {
}

/**
  @function yetAnotherFunc
  @param {yetAnotherFunc~cb} cb - A callback that takes two objects and an array.
 */
function yetAnotherFunc(cb) {
}

Now we have a namepath for each callback that is guaranteed to always be unique and is based on an existing JSDoc pattern. But actually this is starting to look a lot like a previous suggestion in this thread isn't it? How would this look in a more complete example?

/**
  @function someFunc~cb
  @param {boolean} success - Did the asynchronous request succeed?
 */

/**
  @function someFunc
  @param {someFunc~cb} cb - A callback that takes a boolean.
 */
function someFunc(cb) {
}

Starting to look familiar? If it makes you feel better we could make @callback a synonym for @function and we'd get the following:

/**
  @callback someFunc~cb
  @param {boolean} success - Did the asynchronous request succeed?
 */

/**
  @function someFunc
  @param {someFunc~cb} cb - A callback that takes a boolean.
 */
function someFunc(cb) {
}

And, as a bonus, nearly all of this is currently implemented and immediately available already in JSDoc 3, the only thing that needs adding is the @callback synonym, which is trivial to add to the tag dictionary.

@hegemonic, there appears to be a bug in the repo head at the moment that prevents params of global functions from appearing in the output? This makes testing my suggestion difficult, but if you use the -X option you should see that all the data is there in the parsed output.

@cowwoc

micmath,

You incorrectly assume that each callback will have exactly one owner. In some cases, the same callback type will be used by multiple functions. The way I see it, if a callback is used in the scope of a class, then users should be encouraged to prefix their callback names using className~callbackName. If, on the other hand, the callback applies to multiple global functions there is nothing to do other than trying to use a unique prefix.

When users define global functions and class names they run into the same problem. The tool could issue build-time "warnings" if name collisions are discovered and let the user figure it out. The documentation should encourage "best practices" but allow users to use their own patterns if they wish.

@micmath
Owner

@cowwoc, So we may already be very close to a solution: If a generic and reusable callback pattern is wanted I think the @typedef tag could most easily be extended to do that, and then possibly given a synonym of @callback. That tag is already meant to be used to document global custom types that can be reused in multiple doclets. Like @neekey's example it has the feature/problem of allowing arbitrarily named types (so could in theory collide with existing names).

For example:

/**
  @param {requestResponseCallback} cb - A callback is called when a response is received.
 */
function makeGenericRequest(cb) {
}

/**
  @param {requestResponseCallback} cb - A callback is called when a response is received.
 */
function makeSpecialRequest(cb) {
}

/**
    @typedef {function} requestResponseCallback
    @param {number} responseCode
    @param {string} responseText
 */

With the caveat that @callback becomes a synonym for @typedef {function}. The extra bonus is that @typedef is also supported by Google's Closure Compiler, so people wanting to write CC compatible docs, could (in theroy) still do so and continue to use JSDoc as well.

ref:
https://github.com/jsdoc3/jsdoc/blob/master/test/fixtures/typedeftag.js

@hegemonic
Owner

The issue with global function params is now fixed. However, there are other issues that prevent typedefs from appearing correctly in the default template's output; see #292.

Once that issue is fixed, I'll implement the new @callback tag.

@neekey

@hegemonic looking forward to it~

@hegemonic hegemonic was assigned
@hegemonic
Owner

JSDoc now supports a @callback tag, which is a synonym for @typedef {function}. Also, the default template displays callbacks in the HTML output.

Use JSDoc will be updated soon to describe the new tag.

@hegemonic hegemonic closed this
@hydrozen

I'm playing around with this, and I get something like this:

Screen Shot 2013-02-10 at 6 32 52 PM

Seems a bit awkward to have to click the name of the callback to be able to know the arguments it takes, no?

@cowwoc

I don't think you can really do better. As explained earlier, this is a recursive problem.

@slayernyte slayernyte referenced this issue from a commit in slayernyte/jsdoc
@hegemonic hegemonic add '@callback' tag (#260) abd5e77
@mramato mramato referenced this issue in AnalyticalGraphicsInc/cesium
Closed

Standardize callback/function parameter documentation #1085

@Nijikokun

Heh, jsDuck is the best implementation, this is just weird.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.