DevDocs/Proptype Index: Generate slug/displayname and capture inline comments #10343

withinboredom · 2016-12-30T19:33:29Z

While working on #10342, I discovered more than a few components were lacking a "DisplayName" property and React DocGen left it out. We also discovered that there were several components that have inline comments, instead of docblocks, which would be helpful to have.

This PR generates a DisplayName based on the directory name and generates a matching slug. It also captures inline comments and displays some output/timing.

Related to #9919

ehg · 2017-01-04T21:59:08Z

server/devdocs/bin/generate-proptypes-index.js

+		return name;
+	}
+
+	return name


I wonder whether lodash's camelCase and upperFirst suffice here?

Probably -- I stole it from from the dev-docs utils However, I didn't want it to log, so I just copy/pasted because I didn't want to cross server/client code. Maybe I'm being a dinosaur, but that was my reasoning. It may be reasonable to change it to use _.camelCase in both places, or make the util function more reusable. Thoughts?

Ahh. I wonder if we could reuse that module, did it actually work when you tried to import it?

I almost forgot, there's no import in vanilla nodejs, so to use the utility function from the client, it either has to be copy/pasted or the utility module has to have 0 dependencies (or use require).

Maybe a sneaky require( 'babel-register' ) would work?

Good call ... coming right up!

Cool. Does this have any impact on build time?

withinboredom · 2017-01-17T19:23:17Z

server/devdocs/bin/generate-proptypes-index.js


 const root = path.dirname( path.join( __dirname, '..', '..' ) );
 const pathSwap = new RegExp(path.sep, 'g');
+const handlers = [ ...reactDocgen.defaultHandlers, commentHandler ];


I moved handlers out of the loop and to the top of the file ... saved quite a bit of time :)

Nice. But it's still taking quite a long time:

nmda:~/a8c/wp-calypso (fix/devdocs-proptype-index=)$ time make server/devdocs/proptypes-index.json Building: proptypes-index.json Time: 33s 280.235ms

Is this because of the sheer amount of files we're processing? Is there any way we could cut this down?

It is ... It would be nice if we only had to index components in the components or blocks directory.

A further optimization might be to only process files with an "example" component. This might make things quite a bit faster, since we only have to look for them and process them. Most of the time is spent parsing and building I believe.

It is ... It would be nice if we only had to index components in the components or blocks directory.

I would be fine with this. Any component that is reusable should be already be in components or blocks.

A further optimization might be to only process files with an "example" component.

Does a component have to have an example.jsx to be shown at all? If so, I think this is a good idea :)

Showing a message nudging a move to components or blocks, or creating an example.jsx would be cool.

ehg

Cool. I can see theme inline comments in the JSON file now :) Left some other comments, I'm still concerned by build time.

Also when is the JSON file regenerated?

ehg · 2017-01-17T22:30:18Z

.eslintignore

@@ -1,6 +1,7 @@
 .vscode
 assets/
 bin/


I think this needs to change to bin/* in order for the exception for generate-proptypes-index to be respected.

Worth adding this change?

ehg · 2017-01-17T22:30:40Z

server/devdocs/bin/generate-proptypes-index.js

@@ -4,31 +4,97 @@
 * This file generates an index of proptypes by component displayname, slug and folder name


The whole file now needs linting :)

ehg · 2017-01-17T22:38:45Z

server/devdocs/bin/generate-proptypes-index.js


 const root = path.dirname( path.join( __dirname, '..', '..' ) );
 const pathSwap = new RegExp(path.sep, 'g');
+const handlers = [ ...reactDocgen.defaultHandlers, commentHandler ];


Nice. But it's still taking quite a long time:

nmda:~/a8c/wp-calypso (fix/devdocs-proptype-index=)$ time make server/devdocs/proptypes-index.json Building: proptypes-index.json Time: 33s 280.235ms

Is this because of the sheer amount of files we're processing? Is there any way we could cut this down?

ehg · 2017-01-17T22:41:03Z

server/devdocs/bin/generate-proptypes-index.js

@@ -107,6 +178,7 @@ const writeFile = ( contents ) => {
 };

 const main = ( () => {
+	console.log( 'Building: proptypes-index.json' );


Maybe something more descriptive, like "Generating component documentation: …"

withinboredom · 2017-01-19T16:18:57Z

It would be interesting to used marked to add the readme as the description if one isn't set via docblock...

ehg · 2017-02-06T16:00:03Z

server/devdocs/bin/generate-proptypes-index.js

+ * @param {NodePath} nodePath The node path from react-docgen
+ * @return {null|string} The comment or nothing, if no comment
+ */
+function getComments( nodePath ) {


Do you think some tests would elucidate what's going on here a bit better?

Since this file is self-executing, based off of argv there's three ways I can think to make it testable:

Refactor the file into modules that are imported; test the modules -- lots of granularity

execute the script from the test -- more of an integration test, really

all of the above

I'm partial to "all of the above" ... what are your thoughts?

I guess I'd lean towards 1, making generate-prop-types-index a module of its own maybe?

That's what I'm thinking

mcsf

Good stuff here!

I've only looked at this based on the file itself, not considering performance at all, at least for now. My takeaways are that, as this file is growing a bit, striving to make the file clear and easy to read is worth it — if nothing else, for other contributors, but also because it makes it easier to spot eventual inefficiencies, since tackling performance is the next step. Little things like grouping functions by logic (e.g. readFile and writeFile aren't together) and splitting some larger bits (e.g. computing includePath outside of processFile) also help.

mcsf · 2017-02-07T15:32:28Z

server/devdocs/bin/generate-proptypes-index.js

 */
-const createIndex = ( parsed ) => {
+const cleanIndex = ( parsed ) => {


Whenever I see a function like f := x => x.filter( y ) or similar, I ask myself whether it's useful to define f at all, or whether y is best defined on its own, since the predicate y is by nature more reusable. In this case,

const hasDisplayname = ( component ) => { if ( ! component ) { … }

This becomes relevant in main :)

You make a very good point, though IIRC, this function once had much more stuff in it and was cut down to just this singular filter...

mcsf · 2017-02-07T15:55:20Z

server/devdocs/bin/generate-proptypes-index.js

+ * @param {Array} docObjArray The array of docs to filter
+ * @return {Array} The filtered array
+ */
+const onlyWithExample = ( docObjArray ) => {


A name closer to filterDocsWithExample might be more descriptive.

mcsf · 2017-02-07T15:56:18Z

server/devdocs/bin/generate-proptypes-index.js

+	let documents = fileList.map( processFile );
+	documents = onlyWithExample( documents );
+	documents = documents.map( parseDocument );
+	documents = cleanIndex( documents );


This would be a great place for lodash#flow, assuming FP-oriented map and filter:

import { flow } from 'lodash'; import { filter, map } from 'lodash/fp'; const documents = flow( map( processFile ), filterDocsWithExample, map( parseDocument ), filter( hasDisplayName ) )( fileList );

Also, the chain highlights that the process* and parse* functions could better reflect their type signatures in their names, for instance fileToDocument. Alternatively, processFile could be thought of as readDocument.

I swear I learn something new from lodash once a month. That's pretty excellent and simple.

mcsf · 2017-02-07T16:04:36Z

server/devdocs/bin/generate-proptypes-index.js

+		comments = nodePath.node.comments.filter(
+			comment => comment.leading === false
+		);
+	}


Looks like this could be refactored. Off the top of my mind:

if ( leadingComments || comments ) { comments = ( leadingComments || comments ).filter( ( comment ) => comment.leading === !! leadingComments ); }

I don't see how this could work. leadingComments is an array of leading comments. Such that:

do.something(); // this is a leading comment to the next line, but will have comment.leading set to false // this is a leading comment to the next line, but will have comment.leading set to true do.something(); // this is a comment, but will have comment.leading set to false

If you start the method with const { leadingComments, comments } = nodePath.node then those vars will default to undefined if they aren't found in the node. Thus, even though an existing leadingComments would be of type array (eval'ing to true regardless of length), the undefined one would eval to false.

🤦‍♂️ I think I got my PHP and JS mixed up when I first read this. In PHP, [] == false.

mcsf · 2017-02-07T16:07:55Z

server/devdocs/bin/generate-proptypes-index.js

+	}
+
+	return null;
+}


Considering the previous comment, refactoring can go further:

function getComments( nodePath ) { const { leadingComments, comments } = nodePath.node; if ( ! leadingComments && ! comments ) { return null; } const comments = ( leadingComments || comments ).filter( ( comment ) => comment.leading === !! leadingComments ); return parseDocblock( comments[ comments.length - 1 ].value ); }

which incidentally, I feel, better tells us what is happening.

Oh, me likey.

mcsf · 2017-02-07T16:16:18Z

server/devdocs/bin/generate-proptypes-index.js

+function parseDocblock( str ) {
+	const lines = str.split( '\n' );
+	for ( let i = 0, l = lines.length; i < l; i++ ) {
+		lines[ i ] = lines[ i ].replace( /^\s*\*\s?/, '' );


Why the for loop instead of a map? (str.split( … ).map( … ).join( … ).trim())

Interestingly, I originally wrote this against tests I wrote in react-docgen and written there. @ehg and I pulled it out and adapted it for here. The react-docgen code base is mostly for-loops and no es6 features. Hence how this code inherited a for-loop. It was just overlooked. Thanks for spotting it :D

mcsf · 2017-02-07T17:20:48Z

server/devdocs/bin/generate-proptypes-index.js

@@ -106,7 +199,8 @@ const writeFile = ( contents ) => {
 	fs.writeFileSync( path.join( root, 'server/devdocs/proptypes-index.json' ), JSON.stringify( contents ) );
 };

-const main = ( () => {
+( () => {


Technically, this IIFE doesn't add anything except for some visual separation. This is assumedly my personal preference, but I'd keep the named main function, and would perhaps move it to the top (or near the top) of the file, then leave its invocation at the bottom. Having main at the top gives readers an immediate outline of what the program will perform.

mcsf · 2017-02-07T17:23:09Z

server/devdocs/bin/generate-proptypes-index.js

 const fs = require( 'fs' );
 const path = require( 'path' );
 const reactDocgen = require( 'react-docgen' );
+const { getPropertyName, getMemberValuePath, resolveToValue } = require( 'react-docgen/dist/utils' );
+const util = require( 'client/devdocs/docs-example/util' );


util needs an * Internal dependencies block

mcsf · 2017-02-07T17:23:56Z

server/devdocs/bin/generate-proptypes-index.js

 /**
 * External Dependencies
 */
-
+require( 'babel-register' );


Should be above * External dependencies

withinboredom · 2017-02-08T16:44:04Z

@mcsf Thanks for the amazing review!

ehg · 2017-04-18T15:31:37Z

Let's get this merged.

…ine it

withinboredom added Build DevDocs [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 30, 2016

withinboredom self-assigned this Dec 30, 2016

matticbot added the OSS Citizen label Dec 30, 2016

withinboredom mentioned this pull request Jan 3, 2017

DevDocs: Generate a table for displaying proptypes #10342

Closed

withinboredom requested a review from ehg January 3, 2017 20:26

ehg reviewed Jan 4, 2017

View reviewed changes

withinboredom force-pushed the fix/devdocs-proptype-index branch from 542beff to 9d9c762 Compare January 9, 2017 16:42

withinboredom force-pushed the fix/devdocs-proptype-index branch from 9d9c762 to 7ee79fe Compare January 17, 2017 19:19

withinboredom changed the title ~~DevDocs: Generate a DisplayName and Slug for the proptype index~~ DevDocs/Proptype Index: Generate slug/displayname and capture inline comments Jan 17, 2017

withinboredom commented Jan 17, 2017

View reviewed changes

ehg requested changes Jan 17, 2017

View reviewed changes

withinboredom force-pushed the fix/devdocs-proptype-index branch 3 times, most recently from 725ff81 to b970d80 Compare January 18, 2017 18:56

withinboredom force-pushed the fix/devdocs-proptype-index branch 2 times, most recently from 30d47ff to 8b33636 Compare January 30, 2017 14:37

ehg reviewed Feb 6, 2017

View reviewed changes

mcsf reviewed Feb 7, 2017

View reviewed changes

withinboredom force-pushed the fix/devdocs-proptype-index branch 3 times, most recently from 2fac4cb to c44e56f Compare February 21, 2017 03:46

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 21, 2017

withinboredom force-pushed the fix/devdocs-proptype-index branch from c44e56f to 9fc0ffc Compare March 7, 2017 13:52

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 7, 2017

withinboredom force-pushed the fix/devdocs-proptype-index branch from 9fc0ffc to 4f5c84b Compare March 7, 2017 21:21

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 7, 2017

withinboredom force-pushed the fix/devdocs-proptype-index branch from 4f5c84b to 7b1443d Compare March 10, 2017 13:29

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 10, 2017

ehg approved these changes Mar 29, 2017

View reviewed changes

ehg force-pushed the fix/devdocs-proptype-index branch from 7b1443d to 21919dc Compare April 18, 2017 15:22

matticbot added [Size] L Large sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Apr 18, 2017

ehg added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 18, 2017

withinboredom added 5 commits April 19, 2017 09:12

DevDocs: Generate a DisplayName and Slug if the component doesn't def…

a8cc30b

…ine it

DevDocs: Reuse client utils when building proptypes-index.json

5e9b781

Don't ignore this one bin file

f4e975d

Display build time

621133a

DevDocs: Parse inline comments for prop-types

e7b0f57

withinboredom force-pushed the fix/devdocs-proptype-index branch from 21919dc to e7b0f57 Compare April 19, 2017 13:14

withinboredom merged commit 625544c into master Apr 19, 2017

withinboredom deleted the fix/devdocs-proptype-index branch April 19, 2017 13:26

matticbot removed the [Status] Ready to Merge label Apr 19, 2017

withinboredom mentioned this pull request Apr 24, 2017

DevDocs: Display the source code of the example #13216

Closed

@@ @@ -1,6 +1,7 @@ @@
               .vscode
               assets/
               bin/

		@@ -4,31 +4,97 @@
		* This file generates an index of proptypes by component displayname, slug and folder name

DevDocs/Proptype Index: Generate slug/displayname and capture inline comments #10343

DevDocs/Proptype Index: Generate slug/displayname and capture inline comments #10343

Conversation

withinboredom commented Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

withinboredom commented Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

withinboredom commented Feb 8, 2017

ehg commented Apr 18, 2017

withinboredom commented Dec 30, 2016 •

edited

Loading

withinboredom commented Jan 19, 2017 •

edited

Loading

mcsf Mar 29, 2017 •

edited

Loading