Skip to content

Commit

Permalink
Docgen/stop crashing on missing return types (#37929)
Browse files Browse the repository at this point in the history 
In certain situations if `docgen` finds a JSDoc with a description
for the return value of a function but there's no corresponding
type listed for the return value it will error out and say it's required.
This requirement introduces a mismatch with how many TypeScript functions
will be idiomatically _typed_. In this patch we're reporting those missing
types as _unknown_ and letting the `docgen` continue.

When missing return types of default exports `docgen` would silently
crash without an error message because of a bug when trying to find the
name of the surrounding function _when generating the error message_.
This is fixed by special-casing default exports alongside named exports.
  • Loading branch information
@dmsnell
dmsnell committed Jan 24, 2022
1 parent c04a386 commit f663f7a
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 73 deletions.
2 changes: 1 addition & 1 deletion packages/docgen/lib/get-jsdoc-from-token.js
View file
Expand Up @@ -48,7 +48,7 @@ module.exports = ( token ) => {
token,
0
);
if ( potentialTypeAnnotation !== '' ) {
if ( potentialTypeAnnotation ) {
jsdoc.tags.push( {
tag: 'type',
type: potentialTypeAnnotation,
Expand Down
78 changes: 35 additions & 43 deletions packages/docgen/lib/get-type-annotation.js
View file
Expand Up @@ -397,7 +397,10 @@ function getFunctionToken( token ) {

function getFunctionNameForError( declarationToken ) {
let namedFunctionToken = declarationToken;
if ( babelTypes.isExportNamedDeclaration( declarationToken ) ) {
if (
babelTypes.isExportNamedDeclaration( declarationToken ) ||
babelTypes.isExportDefaultDeclaration( declarationToken )
) {
namedFunctionToken = declarationToken.declaration;
}

Expand Down Expand Up @@ -451,7 +454,7 @@ function getQualifiedObjectPatternTypeAnnotation( tag, paramType ) {
* @param {CommentTag} tag The documented parameter.
* @param {ASTNode} declarationToken The function the parameter is documented on.
* @param {number} paramIndex The parameter index.
* @return {null | string} The parameter's type annotation.
* @return {string?} The parameter's type annotation.
*/
function getParamTypeAnnotation( tag, declarationToken, paramIndex ) {
const functionToken = getFunctionToken( declarationToken );
Expand All @@ -469,58 +472,51 @@ function getParamTypeAnnotation( tag, declarationToken, paramIndex ) {
);
}

const isQualifiedName = tag.name.includes( '.' );
if ( babelTypes.isAssignmentPattern( paramToken ) ) {
paramToken = paramToken.left;
}

try {
if ( babelTypes.isAssignmentPattern( paramToken ) ) {
paramToken = paramToken.left;
}
if (
! paramToken.typeAnnotation ||
! paramToken.typeAnnotation.typeAnnotation
) {
return;
}

const paramType = paramToken.typeAnnotation.typeAnnotation;
const paramType = paramToken.typeAnnotation.typeAnnotation;
const isQualifiedName = tag.name.includes( '.' );

if (
babelTypes.isIdentifier( paramToken ) ||
babelTypes.isRestElement( paramToken ) ||
( ( babelTypes.isArrayPattern( paramToken ) ||
babelTypes.isObjectPattern( paramToken ) ) &&
! isQualifiedName )
) {
return getTypeAnnotation( paramType );
} else if ( babelTypes.isArrayPattern( paramToken ) ) {
return getQualifiedArrayPatternTypeAnnotation( tag, paramType );
} else if ( babelTypes.isObjectPattern( paramToken ) ) {
return getQualifiedObjectPatternTypeAnnotation( tag, paramType );
}
} catch ( e ) {
throw new Error(
`Could not find type for parameter '${
tag.name
}' in function '${ getFunctionNameForError( declarationToken ) }'.`
);
if (
babelTypes.isIdentifier( paramToken ) ||
babelTypes.isRestElement( paramToken ) ||
( ( babelTypes.isArrayPattern( paramToken ) ||
babelTypes.isObjectPattern( paramToken ) ) &&
! isQualifiedName )
) {
return getTypeAnnotation( paramType );
} else if ( babelTypes.isArrayPattern( paramToken ) ) {
return getQualifiedArrayPatternTypeAnnotation( tag, paramType );
} else if ( babelTypes.isObjectPattern( paramToken ) ) {
return getQualifiedObjectPatternTypeAnnotation( tag, paramType );
}
}

/**
* @param {ASTNode} declarationToken A function token.
* @return {null | string} The function's return type annoation.
* @return {string?} The function's return type annotation.
*/
function getReturnTypeAnnotation( declarationToken ) {
const functionToken = getFunctionToken( declarationToken );

try {
return getTypeAnnotation( functionToken.returnType.typeAnnotation );
} catch ( e ) {
throw new Error(
`Could not find return type for function '${ getFunctionNameForError(
declarationToken
) }'.`
);
if ( ! functionToken.returnType ) {
return;
}

return getTypeAnnotation( functionToken.returnType.typeAnnotation );
}

/**
* @param {ASTNode} declarationToken
* @return {string} The type annotation for the variable.
* @return {string?} The type annotation for the variable.
*/
function getVariableTypeAnnotation( declarationToken ) {
let resolvedToken = declarationToken;
Expand All @@ -541,7 +537,6 @@ function getVariableTypeAnnotation( declarationToken ) {
return getTypeAnnotation( resolvedToken.typeAnnotation.typeAnnotation );
} catch ( e ) {
// assume it's a fully undocumented variable, there's nothing we can do about that but fail silently.
return '';
}
}

Expand All @@ -550,7 +545,7 @@ module.exports =
* @param {CommentTag} tag A comment tag.
* @param {ASTNode} token A function token.
* @param {number | null} index The index of the parameter or `null` if not a param tag.
* @return {null | string} The type annotation for the given tag or null if the tag has no type annotation.
* @return {[string]} The type annotation for the given tag or null if the tag has no type annotation.
*/
function ( tag, token, index ) {
// If the file is using JSDoc type annotations, use the JSDoc.
Expand All @@ -568,8 +563,5 @@ module.exports =
case 'type': {
return getVariableTypeAnnotation( token );
}
default: {
return '';
}
}
};
74 changes: 54 additions & 20 deletions packages/docgen/lib/markdown/formatter.js
View file
Expand Up @@ -18,7 +18,7 @@ const formatTag = ( title, tags, formatter, docs ) => {
docs.push( '\n' );
docs.push( `*${ title }*` );
docs.push( '\n' );
docs.push( ...tags.map( formatter ) );
docs.push( ...tags.map( ( tag ) => `\n${ formatter( tag ) }` ) );
}
};

Expand Down Expand Up @@ -103,54 +103,88 @@ module.exports = (
formatTag(
'Related',
getSymbolTagsByName( symbol, 'see', 'link' ),
( tag ) =>
`\n- ${ tag.name.trim() }${
tag.description ? ' ' : ''
}${ tag.description.trim() }`,
( tag ) => {
const name = tag.name.trim();
const desc = tag.description.trim();

// prettier-ignore
return desc
? `- ${ name } ${ desc }`
: `- ${ name }`;
},
docs
);
formatExamples( getSymbolTagsByName( symbol, 'example' ), docs );
formatTag(
'Type',
getSymbolTagsByName( symbol, 'type' ),
( tag ) =>
`\n- ${ getTypeOutput( tag ) }${ cleanSpaces(
` ${ tag.name } ${ tag.description }`
) }`,
( tag ) => {
const type = tag.type && getTypeOutput( tag );
const desc = cleanSpaces(
`${ tag.name } ${ tag.description }`
);

// prettier-ignore
return type
? `- ${ type }${ desc }`
: `- ${ desc }`;
},
docs
);
formatTag(
'Parameters',
getSymbolTagsByName( symbol, 'param' ),
( tag ) =>
`\n- *${ tag.name }* ${ getTypeOutput(
tag
) }: ${ cleanSpaces( tag.description ) }`,
( tag ) => {
const name = tag.name;
const type = tag.type && getTypeOutput( tag );
const desc = cleanSpaces( tag.description );

return type
? `- *${ name }* ${ type }: ${ desc }`
: `- *${ name }* ${ desc }`;
},
docs
);
formatTag(
'Returns',
getSymbolTagsByName( symbol, 'return' ),
( tag ) => {
return `\n- ${ getTypeOutput( tag ) }: ${ cleanSpaces(
const type = tag.type && getTypeOutput( tag );
const desc = cleanSpaces(
`${ tag.name } ${ tag.description }`
) }`;
);

// prettier-ignore
return type
? `- ${ type }: ${ desc }`
: `- ${ desc }`;
},
docs
);
formatTag(
'Type Definition',
getSymbolTagsByName( symbol, 'typedef' ),
( tag ) => `\n- *${ tag.name }* ${ getTypeOutput( tag ) }`,
( tag ) => {
const name = tag.name;
const type = getTypeOutput( tag );

return `- *${ name }* ${ type }`;
},
docs
);
formatTag(
'Properties',
getSymbolTagsByName( symbol, 'property' ),
( tag ) =>
`\n- *${ tag.name }* ${ getTypeOutput(
tag
) }: ${ cleanSpaces( tag.description ) }`,
( tag ) => {
const name = tag.name;
const type = tag.type && getTypeOutput( tag );
const desc = cleanSpaces( tag.description );

// prettier-ignore
return type
? `- *${ name }* ${ type }: ${ desc }`
: `- *${ name }* ${ desc }`
},
docs
);
docs.push( '\n' );
Expand Down
14 changes: 5 additions & 9 deletions packages/docgen/test/get-type-annotation.js
View file
Expand Up @@ -36,7 +36,7 @@ describe( 'Type annotations', () => {
};
const node = {};
const result = getTypeAnnotation( tag, node, 0 );
expect( result ).toBe( '' );
expect( result ).toBeFalsy();
} );

const paramTag = {
Expand Down Expand Up @@ -196,16 +196,12 @@ describe( 'Type annotations', () => {
describe( 'missing types', () => {
const node = getMissingTypesNode();

it( 'should throw an error if there is no return type', () => {
expect( () => getTypeAnnotation( returnTag, node, 0 ) ).toThrow(
"Could not find return type for function 'fn'."
);
it( 'should return empty value if there is no return type', () => {
expect( getTypeAnnotation( returnTag, node, 0 ) ).toBeFalsy();
} );

it( 'should throw an error if there is no param type', () => {
expect( () => getTypeAnnotation( paramTag, node, 0 ) ).toThrow(
"Could not find type for parameter 'foo' in function 'fn'."
);
it( 'should return empty value if there is no param type', () => {
expect( getTypeAnnotation( paramTag, node, 0 ) ).toBeFalsy();
} );
} );

Expand Down

0 comments on commit f663f7a

Please sign in to comment.