Problem/Motivation
Our JavaScript comments are currently in a format that neither the API module nor any well-know JS documentation tool understands. This makes it impossible to generate JS documentation the way we generate PHP documentation.
At this time, the main standard JS doc parser is JSDoc, which we can probably integrate with the API module and thus display our JS docs on api.drupal.org... the API module integration is a separate issue: #25901: Parse/save/display JavaScript files
Proposed resolution
This issue is about making sure that our JavaScript comments can be understood by JSDoc, which will incidentally help with IDEs such as PHPStorm. As a test, the patches on the child issues have been used to generate a test Drupal JS API site.
Some more information about JSDoc:
- Using namepaths what does
#
,.
,~
means for JSDoc - Declaring types
See attached screenshot for an example of how the code looks in PHPStorm.
Remaining tasks
a) Needs framework manager feedback - Agree on the strategy for the patches. Two possibilities:
1. nod_ (JS subsystem maintainer) would like to commit the minimal patches that will make sure that all our JS gets picked up by the JSDoc system and that pass some ESLint checks. The JSDoc parser does not make an entry for anything that has no doc block, so the idea is that we should first at least get doc blocks on everything with minimal information for our docs to be usable, and then go back and fix up the docs with more information.
2. jhodgdon (Documentation subsystem maintainer) is not excited about committing patches that do not conform with our docs standards, such as having "@param {type} var_name" not followed by descriptive docs. She is afraid that people will start using these non-conforming doc blocks as templates for future docs, and this problem will be perpetuated. She'd rather see the conforming docs added incrementally, rather than adding non-conforming doc blocks, although she recognizes that because JSDoc only picks up things with doc blocks, this would be a problem.
We need to decide on which approach to take.
b) Once (a) is decided, get the patches on the child issues reviewed and committed.
User interface changes
None. DX is improved if developers eventually have better access to JS docs on api.drupal.org, and IDEs can use them.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#47 | jsdoc.png | 79.2 KB | markhalliwell |
jsdoc.png | 609.01 KB | nod_ |
Comments
Comment #2
nod_Cancelled patch test, it's only changing JS files.
Comment #3
seutje CreditAttribution: seutje commentedHoly 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.
Some of these seem to replace the comment with an empty @file block, or am I missing something?
Does this whitespace change pertain to the JSDoc-ification in particular?
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.
Comment #4
oresh CreditAttribution: oresh commented@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:
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.
2. Not sure why you changed it 'ajax' to 'this', when we defined ajax as this before.
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.
6. No need in @private, as discussed :)
Next 1k lines review coming soon.
Comment #5
nod_Yeah sorry I left some code changes in the patch. I'll leave only the ones that are related to documenting things tonight.
Comment #6
nod_Also JS code standards are not up to date, I added a link to #1778828: [policy, no patch] Update JS coding standards on the doc page to warn people.
Comment #7
nod_Comment #8
jhodgdonWould it be possible to make this a little more similar to our PHP docs standards?
For instance:
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.
Comment #9
nod_Sure I can work the whitespacing to be closer to our PHP standards, not an issue.
It's possible to document what the parameters are (there are a few examples in other files), though they are not without use even like this. The type can be picked up by IDE to validate and give hints on things that can be done with the variable inside the function.
I can't say I'll have every single parameter commented though, there are too many of them to do it in a timely fashion. Patch needs constant reroll and keeping it around for a long time isn't something I look forward to.
Comment #10
jhodgdonPerhaps 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?
Comment #11
webchickWe 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.
Comment #12
jhodgdonWell 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/
Comment #13
nod_Just a reroll because of #1913510: Core Javascript Files Not Using Standard Indentation to avoid really painful merges later on.
Comment #14
Wim Leers#2174589: Split up ckeditor.admin.js has landed and AFAIK was the last blocker to this.
Comment #15
Wim LeersComment #16
YesCT CreditAttribution: YesCT commentedI think since there was a patch (is a patch, but it does not apply) this is needs work... reroll.
Comment #17
nod_Comment #18
jhodgdonFixing title.
A few notes on the previous patch:
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.
I think there should be a blank line before @type too, and generally after the summary line, having a blank line would be good?
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:
Also let's confine this patch to JSDoc and not make other changes like:
Comment #19
nod_Thanks for the feedback. I picked up the patch last week and started rerolling it (and fix the 80 char limit in the process). I'll add all that in.
Side note, there were improvements on jsdoc side, hopefully we won't have to support a custom jsdoc theme or at least it be less involved than what I have now for generating the doc pages.
Comment #20
jhodgdonYeah, 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!
Comment #21
nod_Hold back my great need of refactoring, only touched the comments for the misc folder. I'll continue later on.
JSDoc got a new version out (v3) which makes things better, with the default template and this patch we can have something decent, phpstorm likes it too. Some docs are inconsistent because our code is inconsistent, can't do much about that in this issue. Good first step though.
Comment #22
nod_forgot to put a new line before
/**
in a couple of place.Forgot to say that the patch is fixing the feedback in #18 too
1. Added an empty line after @file docblock
2. Added a new line between doc tag types
3. About the order or tag, haven't thought much about it, always end up doing the same. Will need to look and get a feel of what looks better.
4. All the extra code reformatting and messing with the js code is gone from the patch.
Comment #23
jhodgdonLooking 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.
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:
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.
Formatted is misspelled here.
browser -> browsers
Also I think stack trace is two words?
Should be ID not id. And binded -> bound.
- uesd -> used
- Shouldn't Ajax be capitalized?
- url -> URL
Object should not be capitalized here.
No description/docs?
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.
This is not documenting a JS object or variable, so it should not be a /** comment, just a // comment.
All of these should have docs?
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?
Same here?
Should this just be @code? I saw that earlier in the patch.
Comment #24
nod_I'm moving things along because now most of the code has finished moving around (all the issues about splitting the toolbar, editor module js code in separate JS files are done) it's not crazy to chase HEAD with this patch. As webchick said in #11, it's a first step so while I agree we should document everything, it's just not possible to do in this patch. Right now the documentation is inconsistent and break many rules, sometimes there are
{}
around the variable type, sometimes not, sometimes there is no type or no documentation at all. After the patch things will be consistent, not complete.About the standards, I consider the js CS done, ESlint checks for everything important. As for the doc standards there is a page https://www.drupal.org/node/2183405 it's not completely right (it's 3 years old after all) but it isn't wrong either (and looking back on it, I haven't followed some stuff in my patch), it's from a comment you made in #1337022-63: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser.
@code
doesn't exist in JSDoc, it's@example
.I'll update the patch based on feedback from #23
Comment #25
nod_Some more fixes. I think I got all the places where the first sentence was wrapping and I made it fit.
Comment #26
nod_Added most of our custom events documentation.
Comment #27
nod_Reroll for #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation
Still working only on the
core/misc
folder only.Moved some types around to make the output more handy to read. Added type to all behaviors.
As far as tag order is concerned this order looks pretty good (looked at the PHP one to keep somewhat the same order).
For the typedef group (typedef, var, name, namespace, constructor, callback, event, function) the order inside that group doesn't matter since there should only be one of those tag per docblock maximum.
As for fears of having incomplete docs: ESLint can check for validity of JSDoc comments, once this patch gets in we will turn that rule on, with the settings you can see in the
eslint-jsdoc-do-not-test.patch
. I attached the corresponding output from eslint as an example of running this configuration oncore/misc
after this patch is applied. We can require everything to have a description in a follow-up.Comment #28
jhodgdonI 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?
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!
Comment #29
jhodgdonComment #30
jhodgdonOh 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!
Comment #31
Wim LeersSo 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.
Comment #32
nod_Updated IS.
I believe you're looking for this page of the JSDoc Using namepaths about the ~. (it's not a typo) Basically it's to say the behavior type belongs to the Drupal namespace (then it'll show on the Drupal namespace doc page).
I updated the doc page https://www.drupal.org/node/2183405
I also generated the documentation for the latest version of the patches: Drupal JS API. Contrary to last year, this time I only had to use an existing jsdoc template, change 2 css values and it's already usable. There are a bunch of others but that was the only complete enough with search.
Comment #33
Wim LeersI literally stared at my screen with open mouth at the awesomeness of http://read.theodoreb.net/drupal-jsapi/.
Comment #34
attiks CreditAttribution: attiks at Attiks commentedAmazing work, are we still aiming on integrating it into the php api so we can cross reference?
Comment #35
nod_We can take that up in #1337022: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser and #25901: Parse/save/display JavaScript files (both of which need issue summary updates).
Comment #36
nod_Just uploading the latest fixes, will split that up in the sub issues later.
I added the eslint rule since I fixed all eslint warnings \o/. I did turn off the required description for parameters and returns options because sanity. We will turn that on once this first step gets cleared and we can throw people at it.
Comment #37
nod_Added documentation on a couple of important internal functions. All child issues are up to date, my API doc site has been updated.
Created #2494177: Enable ESLint warning for missing JSDoc to isolate the eslint rule activation patch. I looked at the eslint queue, they're considering adding some more rules around jsdoc, mainly making sure things start with a capital letter and end with a
.
, we'll activate it when it gets in eslint. I guess we can probably contribute to add a rule about tag order and new line formating around those tags.Comment #38
nod_Comment #39
nod_Reroll of child issues following #2488884: Machine name HTML5 validation fails when field is hidden and #2491155: Update drupal.org and kernel.org URLs in core modules (Follow-up to 2489912).
updated http://read.theodoreb.net/drupal-jsapi/
Comment #40
Wim LeersComment #41
nod_See child issues for review.
Comment #42
jhodgdonA 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.
Comment #43
nod_reply to the more general questions from #2493677-8: JSDoc for misc/ files :
My plan was:
It's insane to document all of them now, and if we do it little by little we just can't generate any useful documentation: it's useless to PHPStorm which means nobody will see the documentation and it will take years before the codebase is fixed.
I mean at some point we have to draw a line between being right or being useful. I'm arguing we need to be useful first and that's what my patches reflect. As a Js developer I want the Drupal JS API not to be a black hole. What's best live with, a codebase where, within the same module you have:
Docbock using brackets for type, and no brackets and no description for @param (that's clearly not following anything) or a codebase where everything is marked up but not fully described (yet)? Then we can have novice fill in the blanks, great novice sprint tasks.
Do you want to keep that around for years or would you rather something consistent? I mean It's been like this for ever and nobody ever fixed it. I'm offering a patch that will make the doc visible, useful and that will make people actually read it. Which is how this doc is ever going to be accurate because right now and for the last 9 years, it's been useless.
Comment #44
nod_Updated my jsdoc api site.
Comment #45
nod_If that can help with fears of un-described params and returns I do have a good track record of getting ESLint errors fixed and sticking with unthankful patches #1415788: Javascript winter clean-up.
Comment #46
attiks CreditAttribution: attiks at Attiks commented#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.
Comment #47
markhalliwellI definitely agree with #43.
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:
Comment #48
jhodgdonI 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.
Comment #49
markhalliwellI 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).
Comment #50
jhodgdon@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.
Comment #51
droplet CreditAttribution: droplet commentedI'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.
Comment #52
jhodgdonStandards discussion issue: #1337022: [policy, no patch] Create/adopt JavaScript docs standards compatible with a JS documentation parser
Comment #53
nod_Comment #54
alexpottSo 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.
Comment #55
jhodgdonOK, that's good enough for me. Let's proceed then. Thanks!
Comment #56
nod_Comment #57
nod_Rejoice! Initial patches are in! let's start the clean-up in #2501679: Document JavaScript.
I'll be updating periodically my own jsdoc site http://read.theodoreb.net/drupal-jsapi/ (it's manually updated frequently) until #2501135: Add JSDoc site to api.drupal.org is up and running. Once it's on d.o I'll kill my site.
Comment #59
joseph.olstadHi 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!
Comment #60
joseph.olstad#2501135: Add JSDoc site to api.drupal.org