WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 2 months ago

#41682 closed defect (bug) (fixed)

JSDoc correction for namespaces

Reported by: herregroen Owned by: adamsilverstein
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: javascript, docs Cc:

Description

Currently the generated JSDoc will look as follows: https://atimmer.github.io/wordpress-jsdoc/.

This has the following issues:

  • Everything is in the global namespace.
  • Many functions are incorrectly seen as global functions due to JSDoc not recognising extend.
  • The default JSDoc template will duplicate any namespaces that are defined multiple times.

To fix this the following should happen:

  • Define all used namespaces using @namespace.
  • Correctly specify in which namespace each class is using @memberOf.
  • Define each usage of the extend function as a prototype assignment using @lends.

Attachments (8)

jsdoc-namespaces.patch (201.7 KB) - added by herregroen 3 months ago.
Patch
jsdoc-namespaces.2.patch (202.8 KB) - added by herregroen 3 months ago.
Updated patch
jsdoc-namespaces.3.patch (202.8 KB) - added by herregroen 3 months ago.
Fix typo in class name.
jsdoc-namespaces.4.patch (202.9 KB) - added by herregroen 3 months ago.
Use single quotes in Gruntfile and tabs in jsdoc.conf.json
jsdoc-namespaces.5.patch (203.2 KB) - added by herregroen 3 months ago.
Restore removed comments.
41682.diff (202.1 KB) - added by adamsilverstein 3 months ago.
41682.2.diff (1.2 KB) - added by netweb 2 months ago.
41682.3.diff (640 bytes) - added by netweb 2 months ago.

Download all attachments as: .zip

Change History (41)

@herregroen
3 months ago

Patch

#1 @herregroen
3 months ago

  • Keywords has-patch added

This patch will generate JSDoc that looks as follows: https://herregroen.github.io/wordpress-jsdoc/.

It offers the following improvements:

  • There's a clear overview of namespaces.
  • All classes are children of their respective namespaces.
  • Only actually global functions are part of the global namespace.
  • All class functions are correctly listed under their class.

The following could still be of interest:

  • Add a readme.md to display on the homepage.
  • Change the theme. The template used is https://github.com/docstrap/docstrap which allows for several themes, currently the united theme is chosen. This template was chosen for it's vastly superior namespace support over the default template.
Last edited 3 months ago by herregroen (previous) (diff)

@herregroen
3 months ago

Updated patch

#2 @atimmer
3 months ago

Just for context: I created https://atimmer.github.io/wordpress-jsdoc/ to be an auto-updating representation of the documentation that could be generated from the JavaScript core.

@herregroen has gone through all the documentation in wp-includes and made the namespace structure parsable by jsdoc. This will also make it much easier to parse the output of JSDoc and put it in a WordPress installation.

#3 follow-up: @adamsilverstein
3 months ago

Great work, this is looking really good!

Where would we would include this resource in the handbook? Let's adjust the theme to match the styles there.

This ticket was mentioned in Slack in #core-js by netweb. View the logs.


3 months ago

#5 in reply to: ↑ 3 @netweb
3 months ago

Replying to adamsilverstein:

Great work, this is looking really good!

That it is +1

Where would we would include this resource in the handbook? Let's adjust the theme to match the styles there.

Eventually we'd want this on the https://developer.wordpress.org/ site wouldn't we?

There's also an option to use and adpapt if needed this GitHub to w.org Handbook sync plugin using one of the the JSDoc to Markdown theme templates.

We could also write our own JSDocs templates (not that that I've looked into what this would entail)

#6 @herregroen
3 months ago

I've just updated the preview ( https://herregroen.github.io/wordpress-jsdoc/ ) using an updated copy of DocStrap https://github.com/herregroen/docstrap.

This was simply updating Docstrap to use the latest version of Bootswatch https://bootswatch.com/. The most recent version of Cosmo seemed somewhat WordPress Codex like.

Given that the template does do quite a bit in structuring the menus and adding search functionality I expect the easiest way at least to get close to the codex look would be to base it on our own fork of DocStrap and adapting the styles/main.less file there as needed.

#7 @atimmer
3 months ago

I think the eventual goal should be to have this on developer.wordpress.org just like the parsed PHP code is on there. This will make it possible to have user contributed notes and extra information for functions and namespaces.

The way to get there is by building a convert step from JSDoc to WordPress just like we have for PHP. The PHP work on this can be found on https://github.com/wordpress/phpdoc-parser. I started on a JavaScript version at https://github.com/yoast/jsdoc-parser. This could use all the help it can get. The current idea is to use something like https://www.npmjs.com/package/jsdoc-json to convert the internal JSDoc representation to JSON and use that to create posts in a WordPress site.

This ticket was mentioned in Slack in #core-js by omarreiss. View the logs.


3 months ago

#9 @adamsilverstein
3 months ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#10 @netweb
3 months ago

Related: #3063-meta Show JavaScript documentation on developer.wordpress.org

@herregroen
3 months ago

Fix typo in class name.

#11 @adamsilverstein
3 months ago

Hey @herregroen - thanks for your work here... I was trying to see what you fixed in jsdoc-namespaces.3.patch however it seems to be exactly the same as jsdoc-namespaces.2.patch - are you sure you got that final change saved in this diff?

#12 follow-up: @adamsilverstein
3 months ago

@herregroen I reviewed your patch and have a few questions:

  • do the @lends statements need to be inserted inline like that (and not in the doc block above)? Makes for really long lines and more difficult to read
  • In a few spots you have removed existing inline comments. Are these changes related? can we remove them to keep this patch focused solely on the namespace changes?
  • are parse errors expected when run locally? i did npm install and grunt jsdoc and although many files parsed fine, I also got a bunch of: ERROR: Unable to parse a tag's type expression for source file

#13 in reply to: ↑ 12 ; follow-up: @herregroen
3 months ago

@adamsilverstein

  • The third patch contains a single additional character on a line that's changed in both version ( @class wp.media.collectio -> @class wp.media.collection ).
  • The @lends statements need to be right before the object passed to extend starts. An alternative to reduce line lengths could be:
    Class.extend(
        /** @lends Class.prototype */
        {
            method: function () {
                // Do stuff.
            }
            ... 
  • The comments I've removed were either incorrectly used @global tags ( these do not document a global being used, they document that the function/variable being documented is a global and thus override @memberOf statements ) or comments I felt added no additional information when the correct tags were added ( for example: utility function namespace to @namespace wp.customize.utils )
  • The parse errors are expected. The @global tags I've removed were not the only tags being incorrectly used and JSDoc throws errors for each incorrect usage. The official WordPress javascript documentation standards ( https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/ ) contain some deviations from JSDoc and using them will result in errors. My suggestion here would be to change the javascript documentation standards to match the standards of JSDoc, but that's probably an issue for another ticket. There's also existing documentation that neither follows the JSDoc standards nor the WordPress standards.
Last edited 3 months ago by herregroen (previous) (diff)

#14 in reply to: ↑ 13 ; follow-up: @SergeyBiryukov
3 months ago

Replying to herregroen:

The comments I've removed were either incorrectly used @global tags ( these do not document a global being used, they document that the function/variable being documented is a global and thus override @memberOf statements ) or comments I felt added no additional information when the correct tags were added

I've noticed the patch removes one of the comments added in [33709] for customize-loader.js:

/* 
 * Expose a public API that allows the customizer to be 
 * loaded on any page. 
 */ 

But keeps a similar comment in customize-preview.js:

/* 
 * Script run inside a Customizer preview frame. 
 */ 

Is there a reason for removing the first comment?

#15 @netweb
3 months ago

Are the patches here the same as that of this pull request?

It looks like it is, I've added a comment per @SergeyBiryukov comment above in comment:14 inline in that PR

It might be easier to code review this on GitHub, changes between patches are a little easier to view such as what @adamsilverstein mentioned above in comment:11

#16 follow-up: @netweb
3 months ago

Why are variables being moved to the top of the file? Does JSDoc require this for formatting?

There's quite a few instances of this, for example:

  • src/wp-includes/js/media/controllers/customize-image-cropper.js

     
     1var Controller = wp.media.controller, 
     2        CustomizeImageCropper; 
     3 
    14/** 
    25 * wp.media.controller.CustomizeImageCropper 
    36 * 
     7 * @memberOf wp.media.controller 
     8 * 
    49 * A state for cropping an image. 
    510 * 
    611 * @class 
     
    813 * @augments wp.media.controller.State 
    914 * @augments Backbone.Model 
    1015 */ 
    11 var Controller = wp.media.controller, 
    12         CustomizeImageCropper; 
    13  
    14 CustomizeImageCropper = Controller.Cropper.extend({ 
     16CustomizeImageCropper = Controller.Cropper.extend(/** @lends wp.media.controller.CustomizeImageCropper.prototype */{ 
    1517        doCrop: function( attachment ) { 
    1618                var cropDetails = attachment.get( 'cropDetails' ), 
    1719                        control = this.get( 'control' ), 

Leaving the var declarations where they originally were doesn't cause any issue with linting via JSHint or the proposed ESLint changes we're in the progress of implementing

This ticket was mentioned in Slack in #docs by atimmer. View the logs.


3 months ago

#18 in reply to: ↑ 16 ; follow-up: @herregroen
3 months ago

Replying to netweb:

Why are variables being moved to the top of the file? Does JSDoc require this for formatting?

There's quite a few instances of this, for example:

  • src/wp-includes/js/media/controllers/customize-image-cropper.js

     
     1var Controller = wp.media.controller, 
     2        CustomizeImageCropper; 
     3 
    14/** 
    25 * wp.media.controller.CustomizeImageCropper 
    36 * 
     7 * @memberOf wp.media.controller 
     8 * 
    49 * A state for cropping an image. 
    510 * 
    611 * @class 
     
    813 * @augments wp.media.controller.State 
    914 * @augments Backbone.Model 
    1015 */ 
    11 var Controller = wp.media.controller, 
    12         CustomizeImageCropper; 
    13  
    14 CustomizeImageCropper = Controller.Cropper.extend({ 
     16CustomizeImageCropper = Controller.Cropper.extend(/** @lends wp.media.controller.CustomizeImageCropper.prototype */{ 
    1517        doCrop: function( attachment ) { 
    1618                var cropDetails = attachment.get( 'cropDetails' ), 
    1719                        control = this.get( 'control' ), 

Leaving the var declarations where they originally were doesn't cause any issue with linting via JSHint or the proposed ESLint changes we're in the progress of implementing

JSDoc matches documentation with the line exactly below it.

The comment blocks at the top of files were documenting the var declarations, not the class declaration. This means that the comment block was documenting a class called Controller which has no methods or members of it's own. That's clearly not the intent.

This ties in with the Wordpress Javascript documentation standards leading to errors. A documentation block containing @class should be directly above the class declaration. If the intent is to add a comment block to the top of a file to document what the file contains then this should be done with using @file.

If JSHint or ESLint want documentation blocks at the top of files then these can be added but they should only contain the @file tag and possible the @author tag, there should be no other tags.

#19 in reply to: ↑ 14 ; follow-up: @herregroen
3 months ago

Replying to SergeyBiryukov:

Replying to herregroen:

The comments I've removed were either incorrectly used @global tags ( these do not document a global being used, they document that the function/variable being documented is a global and thus override @memberOf statements ) or comments I felt added no additional information when the correct tags were added

I've noticed the patch removes one of the comments added in [33709] for customize-loader.js:

/* 
 * Expose a public API that allows the customizer to be 
 * loaded on any page. 
 */ 

But keeps a similar comment in customize-preview.js:

/* 
 * Script run inside a Customizer preview frame. 
 */ 

Is there a reason for removing the first comment?

In the places I've added @namespace tags where there were existing comments mentioning exposing the variable, declaring a namespace or anything to that extent I've removed those comments as I personally felt the @namespace tag conveys this information already.

If people feel these comments still add value then I will add them back in.

@herregroen
3 months ago

Use single quotes in Gruntfile and tabs in jsdoc.conf.json

#20 @herregroen
3 months ago

I've uploaded a new patch responding to some reviews from @netweb on GitHub ( see: https://github.com/Yoast/wordpress-develop-mirror/pull/77 ).

#21 in reply to: ↑ 19 @SergeyBiryukov
3 months ago

Replying to herregroen:

If people feel these comments still add value then I will add them back in.

I second @adamsilverstein's comment:

I would prefer all the comments be left in unless they effect the namespace parsing. That way, this patch remains smaller and more focused. We can address these other changes in a follow up ticket if needed.

#22 in reply to: ↑ 18 @netweb
3 months ago

Replying to herregroen:

JSDoc matches documentation with the line exactly below it.

The comment blocks at the top of files were documenting the var declarations, not the class declaration. This means that the comment block was documenting a class called Controller which has no methods or members of it's own. That's clearly not the intent.

This ties in with the Wordpress Javascript documentation standards leading to errors. A documentation block containing @class should be directly above the class declaration. If the intent is to add a comment block to the top of a file to document what the file contains then this should be done with using @file.

Perfect, thanks for the detailed reply +1

If JSHint or ESLint want documentation blocks at the top of files then these can be added but they should only contain the @file tag and possible the @author tag, there should be no other tags.

+1 Let's look at doing this after this has been merged in a new ticket

@herregroen
3 months ago

Restore removed comments.

#23 follow-up: @herregroen
3 months ago

@adamsilverstein

I've just uploaded a patch that restores the few comments that were removed.

#24 in reply to: ↑ 23 @netweb
3 months ago

Replying to herregroen:

@adamsilverstein

I've just uploaded a patch that restores the few comments that were removed.

Thanks, here's that change/commit on GitHub https://github.com/Yoast/wordpress-develop-mirror/pull/77/commits/9cac1b99dd7371249f9fb6a9bdcf06fda4c6697c

This ticket was mentioned in Slack in #core-js by netweb. View the logs.


3 months ago

#27 @adamsilverstein
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41351:

Docs: JSDoc improvements for namespaces.

Improve JS parsing of our inline JSDocs by introducing @namespace, @lends and @memberOf. Helps set the way for showing our JavaScript documentation on developer.wordpress.org, see https://meta.trac.wordpress.org/ticket/3063.

  • Define all used namespaces using @namespace.
  • Correctly specify in which namespace each class is using @memberOf.
  • Define each usage of the extend function as a prototype assignment using @lends.
  • Some comment blocks were moved to correct the parsing of certain definitions.

Props herregroen, atimmer, netweb, SergeyBiryukov.  
Fixes #41682.

@netweb
2 months ago

#28 @netweb
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to add the new file jsdoc.conf.json that was missed in [41351]

@adamsilverstein SVN being SVN ;) -> svn add jsdoc.conf.json

#29 @adamsilverstein
2 months ago

@netweb ha, so many files in my status list I missed it - thanks for catching, will commit soon.

#30 @adamsilverstein
2 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 41370:

Docs: Add jsdoc.conf.json JSDOC configuration file.

Left this file off by mistake in [41351].

Fixes #41682.

@netweb
2 months ago

#31 @netweb
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Patch 41682.3.diff adds the jsdoc folder to .gitignore and svn:ignore

The svn:ignore change is not visible here on Trac but is shown in the diff once downloaded.

Alternatively running svn propedit svn:ignore . and adding jsdoc to the end of the file will do the trick

cc @adamsilverstein

#32 @adamsilverstein
2 months ago

Thanks @netweb, I'll get this committed.

#33 @adamsilverstein
2 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 41381:

JSDocs: add the jsdoc folder to .gitignore and svn:ignore.

Props netweb.
Fixes #41682.

Note: See TracTickets for help on using tickets.