JSDoc for JS using Backbone

Holy shit.

This is such a monumental task, such a big undertaking, that IMHO it'd be fine to commit this patch as-is and then iteratively improve from there. It only touches comments. And it's a big step forward already anyway.

I read the entire thing, and have a few nitpicks, but mostly general questions.

1. +++ b/core/modules/ckeditor/js/ckeditor.drupalimage.admin.js
   @@ -1,9 +1,16 @@
   + * Ckeditor drupalimage admin behavior.
   

   s/Ckeditor/CKEditor/

2. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -1,9 +1,25 @@
   + * Ckeditor JS API.
   

   s/Ckeditor/CKEditor/

   It's not really a JS API, it's just an implementation of the Drupal.editors API!

3. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -1,9 +1,25 @@
        attach: function (element, format) {
   
   @@ -26,6 +42,15 @@
        detach: function (element, format, trigger) {
   
   @@ -40,6 +65,13 @@
        onChange: function (element, callback) {
   
   @@ -50,6 +82,15 @@
        attachInlineEditor: function (element, format, mainToolbarId, floatedToolbarId) {
   

   These should all basically be {@inheritdoc}, if that is possible?

4. +++ b/core/modules/ckeditor/js/ckeditor.stylescombo.admin.js
   @@ -1,3 +1,8 @@
   + * Ckeditor sylecombo admin behavior.
   

   s/Ckeditor/CKEditor/
   s/sylecombo/StylesCombo/

5. +++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
   @@ -199,6 +201,7 @@
   +   * @example
   

   No closing tag?

6. +++ b/core/modules/ckeditor/js/views/KeyboardView.js
   @@ -22,7 +23,7 @@
   -     * {@inheritdoc}
   +     * @inheritdoc
   

   Aha, so it exists? :)

7. +++ b/core/modules/ckeditor/js/views/VisualView.js
   @@ -32,7 +34,12 @@
   +     * @param {string} [value]
   

   What do the square brackets here mean?

8. +++ b/core/modules/editor/js/editor.admin.js
   @@ -178,6 +199,14 @@
   +       * @param {Array} propertyValues
   

   Why is it Array with a capital, but a object without?

9. +++ b/core/modules/editor/js/editor.admin.js
   @@ -664,21 +795,29 @@
   +     * @type {Object.<string, Drupal.FilterStatus>}
   

   Woah, what?

10. +++ b/core/modules/editor/js/editor.formattedTextEditor.js
    @@ -46,14 +67,19 @@
    +     * @inheritdoc
    +     *
    +     * @return {jQuery}
    ...
    +     * @inheritdoc
    +     *
    +     * @param {object} fieldModel
    +     * @param {string} state
    

    If we have @inheritdoc, then why are these additional things necessary?

11. +++ b/core/modules/editor/js/editor.js
    @@ -214,6 +218,19 @@
    +   * Attach editor behaviors to the field.
    
    @@ -240,6 +257,17 @@
    +   * Detach editor behaviors from the field.
    

    "Attaches" & "Detaches". We always use 3rd person singular.

Wim: thanks for reviewing, which I haven't made time for yet, sorry!

One note: in JSDoc, @inheritdoc means "Inherit the all the parent docs". See #1337022-12: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser for a summary of what inheritdoc means for various docs parsers.... Our usage for PHP is like this too.

Anyway, in JSDoc I do not think you can use @inheritdoc and then add other docs. According to JSDoc docs, it shouldn't work right.

RE: Backbone + JSDoc: they wontfixed it: https://github.com/jashkenas/backbone/issues/2203 — :(

I would not recommend using @inheritdoc unless there is actually doc to inherit. And I still do not think that "@inheritdoc + more" is supported in JSDoc.

I looked at the beginning of this patch, which is way too long for a quick review... sorry...

A few questions and ideas:

1. +++ b/core/modules/ckeditor/js/ckeditor.drupalimage.admin.js
   @@ -1,9 +1,16 @@
   + * CKEditor drupalimage admin behavior.
   

   what's "drupalimage"? If it's a name of a class or variable or something, maybe instead of making this separate words, just use the real name of the class/whatever? As it is, it's not really English or code speak.

2. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -1,9 +1,25 @@
   + * CKEditor implementation of {@link Drupal.editors} API.
   

   See like this, this is good, because I immediately know that Drupal.editors is a code thing.

3. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -1,9 +1,25 @@
   +     * @param {HTMLElement} element
   +     * @param {string} format
   +     *
   +     * @return {bool}
   +     */
   

   Need docs on all these.

4. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -40,6 +65,13 @@
   +    /**
   +     *
   +     * @param {HTMLElement} element
   +     * @param {function} callback
   +     *
   +     * @return {bool}
   +     */
        onChange: function (element, callback) {
   

   No description here for the first line, or for the param/returns?

5. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -101,6 +142,9 @@
   +    /**
   +     * @param {object} format
   +     */
        _loadExternalPlugins: function (format) {
          var externalPlugins = format.editorSettings.drupalExternalPlugins;
   

   Um. This isn't really docs?

6. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -117,8 +161,11 @@
   +     * @type {?function}
   

   Is the ? here correct?

+++ b/core/modules/ckeditor/js/ckeditor.stylescombo.admin.js
@@ -46,10 +53,10 @@
-     * @return array

Does it make sense to say at least {Array.

Woah, OK, so second 200K patch review on one day. I am pretty sure I did not get everything, but here is a couple nitpicks. As the other issue, this is probably ready to go, but these small things, since I saw them, could be fixed first, no?

Also, for the record. As far as I could tell, only comments were added / edited, no code was touched. This is a non-disruptive patch in that way.

OK, so my small points:

1. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -40,6 +65,13 @@
   +     * @param {function} callback
   

   Ideally we would use @callback here I guess. But this first spree would probably spawn plenty of these

2. +++ b/core/modules/ckeditor/js/models/Model.js
   @@ -9,26 +9,58 @@
   +       * The configuration for the hidden CKEditor instance that is used to build
   

   81 characters.

3. +++ b/core/modules/editor/js/editor.admin.js
   @@ -11,69 +11,81 @@
   +     * @fires event:drupalEditorFeatureAdded
   

   I was wondering if you were doing any of these. I guess there will be quite some follow-ups on @listens tags. But OK. Just an observation :)

4. +++ b/core/modules/editor/js/editor.admin.js
   @@ -11,69 +11,81 @@
   +     * `Drupal.filterSettingsForEditors[filterID].getRules()`.
   

   Did not research this, but if we had an example to put in as a @see comment, that would be nice. This is a nitpick and could also be its own little issue I guess.

5. +++ b/core/modules/editor/js/editor.admin.js
   @@ -102,22 +114,27 @@
   +       * universe, which shows that it must be allowed to generate the a,
   +       * strong
   +       * and img tags. For the a tag, it must be able to set the "href"
   

   Seems like some reformatting for 80 chars gone wrong :)

6. +++ b/core/modules/editor/js/editor.admin.js
   @@ -593,59 +711,72 @@
   +   * @example <caption>Whitelist the "p", "strong" and "a" HTML tags.</caption>
   

   @example on its own line, caption tag on the following?

7. +++ b/core/modules/editor/js/editor.admin.js
   @@ -593,59 +711,72 @@
   +   * @example <caption>For the "a" HTML tag, only allow the "href" attribute
   

   Same as above

8. +++ b/core/modules/editor/js/editor.admin.js
   @@ -593,59 +711,72 @@
   +   * @example <caption>For all tags, allow the "data-*" attribute (that is, any
   

   ...and again

9. +++ b/core/modules/editor/js/editor.js
   @@ -214,6 +218,19 @@
   +   * @listens event:change
   

   Awesome, there is one of those (mentioned above). There is probably a lot more of those not in your patches (I think I spotted a couple), but that's for other issues.

10. +++ b/core/modules/quickedit/js/models/FieldModel.js
    @@ -178,13 +246,14 @@
    -      // The user is in-place editing this entity, and this field is a candidate
    +      // The user is in-place editing this entity, and this field is a
    +      // candidate
           // for in-place editing. In-place editor should not
    

    I don't get why you changed this. Seems this line used to be 80 chars, but you pushed one word onto a new line, making that the only word on that line?

11. +++ b/core/modules/quickedit/js/models/FieldModel.js
    @@ -224,11 +292,12 @@
    +      //   deed is done. Then: 1) transition to 'inactive' to allow the field
    +      // to
    +      //   be rerendered, 2) destroy the FieldModel (which also destroys
    

    another instance of "one word on one line"

12. +++ b/core/modules/quickedit/js/quickedit.js
    @@ -82,23 +86,25 @@
    +      // immediately. New fields will be unable to be processed immediately,
    +      // but
    +      // will instead be queued to have their metadata fetched, which occurs
    

    ...and here

13. +++ b/core/modules/quickedit/js/theme.js
    @@ -10,10 +10,11 @@
    +   *   the id to apply to the backstage.
    

    Super extreme nitpick here, but this should probably use sentence case.

14. +++ b/core/modules/quickedit/js/views/EditorView.js
    @@ -7,36 +7,35 @@
    +     * Drupal.quickedit.EditorView.prototype.initialize.call(this, options);
    

    New line after @example?

Again, this is awesome work @nod_!

All the feedback from @eiriksm got adressed

Thanks for the patches/reviews!

Follow-ups that I noticed, besides the generic "fill this in better":

core/modules/editor/js/editor.admin.js:
- Needs attention for line wrapping/formatting
- Lots of doc blocks have paragraphs of description but lack a one-line summary at the start
- Does JSDoc support numbered lists? I know that the API module doesn't, so comments like:

+   *  1. use the "tags" key to list HTML tags, and set the "allow" key to
+   *     either true (to allow these HTML tags) or false (to forbid these HTML
+   *     tags). If you leave the "tags" key's default value (the empty array),
+   *     no restrictions are applied.
+   *  2. all nested within the "restrictedTags" key: use the "tags" subkey to
+   *     list HTML tags to which you want to apply property restrictions, then
+   *     use the "allowed" subkey to whitelist specific property values, and
+   *     similarly use the "forbidden" subkey to blacklist specific property
+   *     values.

will not format correctly. Should check the JSDoc output and see if this works. Probably this one would actually be better as a bullet list anyway, because it's not really ordered information. (This is in the docs for Drupal.FilterHTMLRule by the way).

core/modules/editor/js/editor.js
- There seem to be a number of doc blocks without the @constructor, @type, or other indication of what exactly they are. Possibly I'm wrong about this, but I thought you had to put something in the doc block to tell JSDoc what is being documented, or it wouldn't pick it up?
- I noticed @prop there, which I guess is a legal synonym of @property for JSDoc. But I think elsewhere in the JS codebase we're using @property. It would be nice to pick one and standardize I think within Drupal, as much as possible?

Generically:

I find it really confusing to see something like this:

+     * @param {string} from
+     * @param {string} to
+     * @param {object} context
+     * @param {string} context.reason
+     * @param {bool} context.confirming
+     *
+     * @return {bool}
+     *
+     * @see Drupal.quickedit.AppView#acceptEditorStateChange
+     */
     _acceptStateChange: function (from, to, context) {

The function parameters are from, to, and context. The lines like @param context.reason are telling us about properties of the context object. Is this really the standard way to do that? It seems very very very odd to me.

In PHP land, we'd do something like this:

* @param object $context
*    An object with the following properties:
*    - reason: The reason for ...

Anyway... Committed as-is to 8.0.x. Thanks everyone for the patches and reviews!

Wohoo!

The function parameters are from, to, and context. The lines like @param context.reason are telling us about properties of the context object. Is this really the standard way to do that? It seems very very very odd to me.

In PHP land, we'd do something like this:

It is the standard, yes. Also, personally I find it better, expecially the output when you are documenting like that. The above example outputs this, for example:

This tells me (in the same way it describes the parameters) that the object consists of those properties, with those types.

Granted, it is not ideal that this is not the same as the php way, and also, I guess this is somewhat a matter of taste as well.

Anyway, great work to everyone involved, and especially @nod_! :)

Good enough! We should not try to shoehorn JS into PHP standards necessarily, and as long as JSDoc is happy that is good enough. Thanks for the explanation.