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

[api-documenter] Print out tag name for unknown block tags #1296

Merged
merged 6 commits into from
Jun 26, 2019

Conversation

raymondfeng
Copy link
Contributor

We ran into various cases that tsdoc comments have unknown block tags, such as:

/**
 * @decorator A new tag
 * @return A bad tag
 */

Without this PR, api-documenter just aborts with Unsupported element kind: BlockTag. The error message is meaningless and it does not help fix the underlying issue.

I would also like to know what source TS file (line/column) has the invalid tag. Please let me know how to access such metadata so that I can improve this PR.

@raymondfeng raymondfeng changed the title Print tag name Print out tag name for unknown block tags May 25, 2019
@octogonz octogonz changed the title Print out tag name for unknown block tags [api-documenter] Print out tag name for unknown block tags May 26, 2019
@octogonz
Copy link
Collaborator

api-documenter definitely should not be crashing for these inputs. We should fix that in this PR, at the very least.

But I'm not sure that the documentation tool render these blocks. For a commercial company, it can be embarrassing to have unprofessional get rendered on your public website. As a general policy, when we encounter an unrecognized TSDoc tag, it's safer to omit it than to render it.

Some specific thoughts on that:

  • For trivial misspellings (@return instead of @returns), TSDoc is planning a "lax mode" that could support synonyms for tags. This is not much work, if someone wants it.

  • For genuinely undefined tags, I'm thinking maybe they should get parsed as blocks, which would cause them to get omitted (since api-documenter ignores unrecognized block types). I'm proposing this change for the TSDoc parser in RFC: Support for custom TSDoc tags tsdoc#21 (comment) -- lemme know your thoughts on this design change.

  • If your scenario is that you want to define custom tags for some special purpose, there's an existing issue [api-extractor] Support extensible AEDoc tags #519 that would allow them to be defined in api-extractor.json. We can prioritize it if you like. (The only reason it didn't get implemented yet is that nobody has asked.)

@octogonz octogonz added the PR: Waiting for author We reviewed your PR but had questions/suggestions. Let us know when you're ready for another look. label May 26, 2019
@raymondfeng
Copy link
Contributor Author

@octogonz Thank you for the input.

In my case, the first step is to make sure unknown tags do NOT crash api-documenter. We should warn that such tags are ignored so that they can be fixed if it's there by mistake. Let me improve the PR to not throw an error.

@octogonz
Copy link
Collaborator

In my case, the first step is to make sure unknown tags do NOT crash api-documenter.

Agreed

We should warn that such tags are ignored so that they can be fixed if it's there by mistake.

Doesn't the tsdoc-undefined-tag warning catch it already?

@raymondfeng
Copy link
Contributor Author

@octogonz Can you elaborate tsdoc-undefined-tag? Is it a configurable option?

@octogonz
Copy link
Collaborator

If you paste your comment into the TSDoc playground, you should see that warning being reported. And the default api-extractor.json should report it (messages.tsdocMessageReporting setting).

@halfnibble
Copy link
Contributor

tsdoc may be showing a warning, but it looks like api-documenter throws an error by default.
This PR appears to address that issue and with the latest changes, it looks good to me.

@halfnibble halfnibble self-requested a review June 21, 2019 05:21
@octogonz octogonz removed the PR: Waiting for author We reviewed your PR but had questions/suggestions. Let us know when you're ready for another look. label Jun 23, 2019
@octogonz
Copy link
Collaborator

Agreed, looks like @raymondfeng already pushed a fix that addresses my concern above. I'll get this merged.

default:
throw new Error('Unsupported element kind: ' + docNode.kind);
console.warn('Unsupported element kind: ' + docNode.kind);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@halfnibble This change was not safe in my opinion: If API Documenter does not recognize some part of the TSDoc syntax tree, that's a bug. We would want people to report an error, so we can determine a correct handling of that node.

Whereas if API Documenter silently ignored the unrecognized node, then it would be simply omitted from the output. It could be years before we eventually realized that we forgot to implement it. Note that it's okay to ignore an unsupported tag; here I'm referring to a generalized node in the tree.

This concern is particularly important since currently API Extractor / API Documenter are serving as the reference implementation of the TSDoc standard.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I pushed a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants