Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use jsdoc aliases if visible when printing types #24153

Merged
merged 4 commits into from
May 16, 2018

Conversation

weswigham
Copy link
Member

Fixes #24140

This avoids deeply structurally printing massive types which may have been defined in jsdoc. 😉

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This needs to handle @callback too.
  2. getEffectiveTypeParameterDeclarations needs a better implementation for jsdoc type aliases.

@@ -4024,6 +4024,10 @@ namespace ts {

function determineIfDeclarationIsVisible() {
switch (node.kind) {
case SyntaxKind.JSDocTypedefTag:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to handle SyntaxKind.CallbackTag too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#23947 isn't merged yet

node.kind === SyntaxKind.ClassExpression || node.kind === SyntaxKind.TypeAliasDeclaration) {
const declaration = <InterfaceDeclaration | TypeAliasDeclaration>node;
node.kind === SyntaxKind.ClassExpression || node.kind === SyntaxKind.TypeAliasDeclaration || node.kind === SyntaxKind.JSDocTypedefTag) {
const declaration = <InterfaceDeclaration | TypeAliasDeclaration | JSDocTypedefTag>node;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same. There is a predicate isJSDocTypeAlias that should maybe just be isTypeAlias and include SyntaxKind.TypeAliasDeclaration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#23947 isn't merged yet

@@ -3093,11 +3093,13 @@ namespace ts {
* Gets the effective type parameters. If the node was parsed in a
* JavaScript file, gets the type parameters from the `@template` tag from JSDoc.
*/
export function getEffectiveTypeParameterDeclarations(node: DeclarationWithTypeParameters) {
return node.typeParameters || (isInJavaScriptFile(node) ? getJSDocTypeParameterDeclarations(node) : undefined);
export function getEffectiveTypeParameterDeclarations(node: DeclarationWithTypeParameters | JSDocTypedefTag) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a few other places that we get type parameter declarations of typedef and callback tags. They explicitly do not use getEffectiveTypeParameterDeclarations because it incorrectly (1) finds the host and (2) looks for @template in any jsdoc comment. Jsdoc type aliases only look for @template in their containing comment.

That said, it's probably the Right Thing to make getEffectiveTypeParameterDeclarations handle jsdoc type aliases, and have everybody call it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, it's probably the Right Thing to make getEffectiveTypeParameterDeclarations handle jsdoc type aliases, and have everybody call it.

Do you not have such a fix in #23947 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, along with the astonishing ability to forget what I did two weeks ago!

@weswigham
Copy link
Member Author

@sandersn Two of your comments don't seem to apply until your PR adding callback tag support is merged? How's this look as is?

@sandersn
Copy link
Member

Well, good, except that 50% of the code will have to change after it's merged and will have lurking type parameter lookup bugs if it's not. If you merge this, can you open a bug either on me or on you explaining that @callback won't have this fix, once it's merged?

@weswigham
Copy link
Member Author

weswigham commented May 16, 2018

I'm not particularly inclined to duplicate your work fixing jsdoc alias type parameters (which'd have issues merging anyway), nor can I implement the fixes for @callback until we actually support @callback; so notes on you it is!

@weswigham weswigham merged commit 5bf6e30 into microsoft:master May 16, 2018
@weswigham weswigham deleted the use-jsdoc-aliases branch May 16, 2018 19:58
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants