[Meta] Document Drupal JavaScript using JSDoc

Holy moley this thing is huge, nearly crashes my Chromesie when loading it up in Dreditor...

I only managed to scroll through about a third of it so far, so consider this partial, at best.

1. @@ -1,6 +1,7 @@
    /**
   - * Limits the invocations of a function in a given time frame.
   - *
   + * @file
   + */
   +/**
   

   Some of these seem to replace the comment with an empty @file block, or am I missing something?

2. +++ b/core/misc/ajax.js
   @@ -1,28 +1,29 @@
    
   -    function loadAjaxBehavior(base) {
   +    function loadAjaxBehavior (base) {
          var element_settings = settings.ajax[base];
   

   Does this whitespace change pertain to the JSDoc-ification in particular?

3. +++ b/core/misc/dropbutton/dropbutton.js
   @@ -36,21 +41,29 @@ function dropbuttonClickHandler (e) {
   + *   visual UAs.
     */
   -function DropButton (dropbutton, settings) {
   +Drupal.DropButton = function (dropbutton, settings) {
      // Merge defaults with settings.
   

   Sneaking in some secret optimalisations? All fine by me, but our test coverage isn't all that, so something this big will need quite a bit of testing.

I'll try to run through the rest later today or somewhere this week.

@seutje, about the spaces before brackets. I think nod_ is trying to follow js coding standards but sometimes it's just personal preferences :).
As told here:

There should be no space between the function name and the following left parenthesis.
Exception: If a function literal is anonymous, there should be a single space between the word "function" and the left parenthesis "(". Otherwise, it can appear that the function's name is actually "function".

About your .1 - I also saw that missing line, but it's declaed a bit lower in @summary or something like that :)

My results of first 1k lines.

1. I'm not sure if this is a jQuery object. In function parameters in comment it says: @param {HTMLElement} element. A bit lower there's this.$form which is a jQuery object.

+++ b/core/misc/ajax.js
@@ 129
+ * @param {Object} element_settings
  */
 Drupal.ajax = function (base, element, element_settings) {
...
@@ 159
+  /**
+   * @type {jQueryObject}
+   */
   this.element = element;
...
@@ 172
+     * @type {jQueryObject}
+     */
     this.$form = $(this.element.form);

2. Not sure why you changed it 'ajax' to 'this', when we defined ajax as this before.

+++ b/core/misc/ajax.js
@@ 213
  var ajax = this;
-  ajax.options = {
...
@@ 230
+  this.options = {

3. Drupal.autocomplete is not commented in misc/autocompete.js

4. Drupal.behavior.collapse misses description. Something like:
"Adds html5 details element functionallity/behavior for old browsers"

5. Function comment is broken one.

+++ b/core/misc/dialog.js
@@ 603
+  /**
+   * When using this API directly (when generating dialogs on the client side),
+   * you may want to override this method and do
+   * as well, to remove the dialog on closing.
+   * @example

6. No need in @private, as discussed :)

+++ b/core/misc/displace.js
@@ 770
+   * @private
+   * @function
+   * @return {OffsetObject}

Next 1k lines review coming soon.

Would it be possible to make this a little more similar to our PHP docs standards?

For instance:

 /**
  * Extends Error to provide handling for Errors in AJAX
+ * @constructor
+ * @augments Error
+ * @param {xhr} xmlhttp
+ * @param {String} uri
  */
 Drupal.AjaxError = function(xmlhttp, uri) {

Normally in our PHP docs we have blank lines between each different type of @ tag. Also, we require that parameters actually describe what they are -- I believe JSDoc support this? These @param descriptions are not actually documentation -- they are just repeating what you can see in the function declaration.

Perhaps before this patch goes in, we should finalize the standards then? And then rather than one big patch that attempts to comment all the JS it should be split into several smaller issues that are not so hard to reroll?

We do not historically have a good track record at all of landing multiple smaller issues in anything approaching a timely manner. Now that we do "destabilization days" following an alpha, I'd personally rather get this in as one big patch and do further clean-ups later. Once the code exists in HEAD it's much easier to clean up.

Agreed though that we should get the details nailed down before commit so that the change notice is accurate and can be pointed to in Novice issues to perform further clean-ups.

Also, this feels at least major, if not critical, to me. We currently have zero documentation of JS on api.d.o from what I understand, and while getting that in place is a separate problem/issue, given how much more JS there is in D8 than any release previous, this patch is a rather big deal IMO.

Well I don't think we can know how to review this patch to see if it complies with our JS standards until we have JS standards for docs.

We have two issues:
#1337022: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser
#1778828: [policy, no patch] Update JS coding standards

On one of those issues we need to decide what the standards are and adopt them, and then we can return here and make/review/commit a patch/

#2174589: Split up ckeditor.admin.js has landed and AFAIK was the last blocker to this.

I think since there was a patch (is a patch, but it does not apply) this is needs work... reroll.

Fixing title.

A few notes on the previous patch:

/**
+ * @file
+ * Provides Ajax page updating via jQuery `$.ajax`.
+ *
+ * Ajax is a method of making a request via JavaScript while viewing an HTML
+ * page. The request returns an array of commands encoded in JSON, which is
+ * then executed to make any changes that are necessary to the page.
+ *
+ * Drupal uses this file to enhance form elements with #ajax['path']
+ * and #ajax['wrapper'] properties. If set, this file will automatically be
+ * included to provide Ajax capabilities.
+ */
 (function ($, window, Drupal, drupalSettings) {

A @file doc block should always have a blank space after it, or else most docs parsers would think the doc block is documenting the next line of code.

   /**
    * Attaches the Ajax behavior to each Ajax form element.
+   * @type {Behavior}
    */
   Drupal.behaviors.AJAX = {

I think there should be a blank line before @type too, and generally after the summary line, having a blank line would be good?

   /**
    * Extends Error to provide handling for Errors in AJAX
+   * @constructor
+   * @augments Error
+   * @param {xhr} xmlhttp
+   * @param {String} uri
    */
   Drupal.AjaxError = function (xmlhttp, uri) {

It would be good to establish an order for the @tags in the doc blocks, even if JSDoc doesn't require it, for consistency. And to leave blank lines between different types of @tags, for human readability. In our PHP docs standards, we have similar standards.

There should also be a blank line before each doc block, like:

     this.message = statusCode + pathText + statusText + responseText + readyStateText;
+    /**
+     * Set to `AjaxError`, used by some browser to display a more accurate
+     * stacktrace.
+     * @type {String}
+     */
     this.name = 'AjaxError';

Also let's confine this patch to JSDoc and not make other changes like:

-    } catch (e) {}
+    }
+    catch (e) {}

Yeah, that would be good! The issue for adding JS parsing to the API site is:
#25901: Parse/save/display JavaScript files
(already listed as "related"). If you have any updates on how to get that to happen, that would be the place to add them. Thanks!

Looking better!

So... It's pretty clear that we need JSDoc standards, and I would hope they would be in line with the PHP standards we already have. The issues establishing the standards are not finalized, so I'm not sure why we're proceeding with this patch already... so I've evaluated this based on an assumption that any JS standards we adopt would be in line with the PHP standards we have (plus or minus differences between the languages).

Here are some notes -- I didn't get through the whole patch, but most of them apply in multiple places.

1. +++ b/core/misc/ajax.js
   @@ -1,19 +1,22 @@
   +/**
   + * @file
   + *
   + * Provides Ajax page updating via jQuery $.ajax (Asynchronous JavaScript and
   + * XML).
   + *
   

   Would it be possible to follow the general PHP doc block and @file block standards we have had in place for years?

   So here, it would be:

   /**
    * @file
    * One line description.
    *
    * Rest of stuff.
   

2. +++ b/core/misc/ajax.js
   @@ -81,6 +85,14 @@
   +   * @param {XMLHttpRequest} xmlhttp
   +   *   xhr object.
   +   * @param {string} uri
       */
      Drupal.AjaxError = function (xmlhttp, uri) {
   

   What is an "xhr object"? I truly have no idea what it is.

   Normally in docs, acronyms would be all caps too, so it would be "XHR" presumably?

   The second param also is lacking docs.

   In this case, I think it would be sensible to have something like "The URI where the error occurred" or whatever the URI is, for the doc line.

3. +++ b/core/misc/ajax.js
   @@ -125,7 +138,18 @@
   +     * Formated and translated error message.
   

   Formatted is misspelled here.

4. +++ b/core/misc/ajax.js
   @@ -125,7 +138,18 @@
   +     * Used by some browser to display a more accurate stacktrace.
   

   browser -> browsers

   Also I think stack trace is two words?

5. +++ b/core/misc/ajax.js
   @@ -142,6 +166,16 @@
   +   *   id of element the event is binded to.
   

   Should be ID not id. And binded -> bound.

6. +++ b/core/misc/ajax.js
   @@ -142,6 +166,16 @@
   +   *   DOM element uesd to find the ajax url.
   

   - uesd -> used
   - Shouldn't Ajax be capitalized?
   - url -> URL

7. +++ b/core/misc/ajax.js
   @@ -142,6 +166,16 @@
   +   *   Settings for the Drupal Ajax Object.
   

   Object should not be capitalized here.

8. +++ b/core/misc/ajax.js
   @@ -180,15 +217,30 @@
   +      /**
   +       * @type {string}
   +       */
          this.wrapper = '#' + this.wrapper;
        }
   

   No description/docs?

9. +++ b/core/misc/ajax.js
   @@ -180,15 +217,30 @@
   +    /**
   +     * @type {HTMLElement}
   +     */
        this.element = element;
   +
   

   No docs here. Do we have standards now? In PHP docs land we require a description on every doc block at a minimum. Should have the same requirement for JS.

10. +++ b/core/misc/ajax.js
    @@ -200,33 +252,56 @@
    +    /**
    +     * Replacing 'nojs' with 'ajax' in the URL allows for an easy method to let
    +     * the server detect when it needs to degrade gracefully.
    +     * There are four scenarios to check for:
    +     * 1. /nojs/
    +     * 2. /nojs$ - The end of a URL string.
    +     * 3. /nojs? - Followed by a query (e.g. path/nojs?destination=foobar).
    +     * 4. /nojs# - Followed by a fragment (e.g.: path/nojs#myfragment).
    +     */
         this.url = this.url.replace(/\/nojs(\/|$|\?|#)/g, '/ajax$1');
     
    

    This is not documenting a JS object or variable, so it should not be a /** comment, just a // comment.

11. +++ b/core/misc/ajax.js
    @@ -200,33 +252,56 @@
    +     * @prop {String} url
    +     * @prop {Object} data
    +     * @prop {Function} beforeSerialize
    +     * @prop {Function} beforeSubmit
    +     * @prop {Function} beforeSend
    +     * @prop {Function} success
    +     * @prop {Function} complere
    +     * @prop {String} dataType
    +     * @prop {Object} accepts
    +     * @prop {String} accepts.json
    +     * @prop {String} type
    +     */
         ajax.options = {
           url: ajax.url,
    

    All of these should have docs?

12. +++ b/core/misc/ajax.js
    @@ -507,7 +583,8 @@
       /**
    -   * Build an effect object which tells us how to apply the effect when adding new HTML.
    +   * Build an effect object which tells us how to apply the effect when adding
    +   * new HTML.
        */
       Drupal.ajax.prototype.getEffect = function (response) {
         var type = response.effect || this.effect;
    

    Our PHP docs standards require the first line after a /** to be one complete sentence ending in . and less than 80 chars, and then any other docs to be a new paragraph. Presumably we should do the same in JS?

13. +++ b/core/misc/ajax.js
    @@ -557,10 +634,14 @@
       /**
    -   * Provide a series of commands that the server can request the client perform.
    +   * Provide a series of commands that the server can request the client
    +   * perform.
    +   *
    +   * @constructor
        */
       Drupal.AjaxCommands = function () {};
       Drupal.AjaxCommands.prototype = {
    

    Same here?

14. +++ b/core/misc/announce.js
    @@ -1,9 +1,12 @@
    + * @example
    

    Should this just be @code? I saw that earlier in the patch.

I was going to review the child issues, but I am not familiar enough with the JSDoc standards that apply. Like, what does this line mean?

+   * @type {Drupal~behavior}
   */
   Drupal.behaviors.activeLinks = {
 

I have no idea what this means, or what the proper syntax is, so it is difficult for me to review it... some other @type lines just say {string}, but this one has a ~ in it, hum... not sure what it means.

Then I went to this issue's summary and also didn't find any links to the proposed standards that the docs are supposed to comply with or what the tags mean. I'm also not clear for the child issues on exactly which docs standards the patches are supposed to be fixing and which ones are being left for later issues.

So can we get a quick issue summary update on this issue with links to the standards, and on the child issues to tell me what I'm supposed to be looking for and what I should let slide in my review? Thanks!

Oh I guess the child issues do tell me what I'm supposed to review for. I just need info on the JSDoc ins and outs and our standards. Thanks!

So much good stuff here. Amazing work, nod_!

Also very cool that ESLint is able to check the validity of JSDoc comments :) Soon, we'll be wishing we had PHPLint :)

@jhodgdon Regarding that tilde sign: I suspect it's a typo, because it's not mentioned here either: http://usejsdoc.org/tags-type.html.

I literally stared at my screen with open mouth at the awesomeness of http://read.theodoreb.net/drupal-jsapi/.

Amazing work, are we still aiming on integrating it into the php api so we can cross reference?

A general comment:

I would much much much rather see a smaller patch that really fixes up the docs for a smaller number of things, rather than a big patch that is really difficult to review (on account of size) and doesn't fully fix up the docs. It's hard to commit docs that are not following our own standards, like adding @param without a description -- I'd rather just leave it out until someone has time to add a description instead of adding in a non-compliant doc block.

#42 For what it is worth I agree with #43, it might not be following the guidelines, but at least it will provide us with something that we can use.

I definitely agree with #43.

It's hard to commit docs that are not following our own standards, like adding @param without a description

That's because these are http://usejsdoc.org standards, not "ours" per se. Also, for IDEs like PHPStorm, having a @param with no description is far better than no @param at all. It doesn't show you the description in autocomplete anyway.

There's a screenshot in the issue summary of why this issue is important, but I'll create another one to see if it helps clear it up a little better:

I understand that an @param with a type and no description is better than not having an @param. My worry is just that these look like complete doc blocks to people and they'll be copy-pasting them as templates, and that the incomplete docs will be propagated. So I'm generally against committing wholesale "improve the docs" patches that add non-compliant docs.

I think we should get the opinion of a decision-maker on this, and I don't think I am the final decision-maker on this, because it affects both the JavaScript and Documentation subsystems. So we need the Framework Manager to weigh in, following the logic in #2457875: [policy] Evolving and documenting Drupal core's structure, responsibilities, and decision-making. [The current framework managers are effulgentsia, catch, and alexpott.]

Adding tag, and adding note to issue summary about what decision needs to be made. Actually, updating the whole issue summary and adding a template.

I was going to say a whole lot more on this, but it's just not worth it anymore. Suffice it to say, some of it is in this comment #1337022-100: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser.

Btw, I also find it rather tacky and a moot point writing "@nod_'s argument" for him (which is mostly negated and generally steers in the direction of #2 anyway).

@markcarver: I figured that if @nod_ disagreed with what I wrote, he'd update the issue summary, although I guess I didn't say that explicitly. Sorry if I stepped on toes, I was just trying to keep things moving and avoid stalling everything for forever. You seem to be kind of angry or disappointed or something... I hope we can keep the discussion constructive here, figure out what's best to do, and get it done.

I'm not worrying about `no description`. Instead, I'm more worrying about if we make it *mandatory* policy, we will adding meaningless comments, eg "A string is String".

While documenting files is not popular in JS world, I could imagine any extra *crazy* Drupal Rule will kick contributors out of Drupal Community.

Standards discussion issue: #1337022: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser

So I'm not sure that this really is a framework manager issue. Afaics we have a disagreement about the approach to take between @nod_ (JS maintainer) and @jhodgdon (Docs maintainer).

Personally (non-framework manager hat on), I think given the scale of the changes that we need to do to fully implement a JSDoc standard, it would be good to go with @nod_'s proposal because then we get a working API site and people will be encouraged to file patches to flesh out the information. I understand @jhodgdon's concerns - we do do a lot of c&p errors with PHP but doing this incrementally will allow us to make quicker progress and settle the standards whilst using them - so we'll have less theoretical discussions.

OK, that's good enough for me. Let's proceed then. Thanks!

Hi nod_ , thanks for keeping your site up, it is very easy to read. I did still have to dig a bit to figure out how to do what I needed to do, the api docs were very helpful. Thanks!

http://read.theodoreb.net/drupal-jsapi/

It's been 4 years, we should consider moving this to api.drupal.org at some point, meanwhile, thanks for maintaining this @_nod ! Merci!

#2501135: Add JSDoc site to api.drupal.org