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

Treat link tag as comment #25206

Merged
merged 3 commits into from
Jun 25, 2018
Merged

Treat link tag as comment #25206

merged 3 commits into from
Jun 25, 2018

Conversation

sandersn
Copy link
Member

After some discussion we decided that the best approach for the @link tag is stop trying to parse it at all. This PR parses it as a comment instead. This trivially makes @link inline, but it's no longer a tag.

Of course, there is no name resolution capability as requested in #16498. However, it does make the quick info formatting correct (as desired in a recent spate of PRs on Definitely Typed [1]), and lets VS code apply its own formatting as much as possible.

[1] DefinitelyTyped/DefinitelyTyped#26674
DefinitelyTyped/DefinitelyTyped#26524
DefinitelyTyped/DefinitelyTyped#26452
DefinitelyTyped/DefinitelyTyped#26449

But my machine is dying, so this is an emergency commit.
But it's not right yet; the test I added fails. See the TODO I added.
case SyntaxKind.OpenBraceToken:
state = JSDocState.SavingComments;
if (lookAhead(() => nextJSDocToken() === SyntaxKind.AtToken && tokenIsIdentifierOrKeyword(nextJSDocToken()) && scanner.getTokenText() === "link")) {
pushComment(scanner.getTokenText());
Copy link
Member

Choose a reason for hiding this comment

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

Does this handle {@link} (malformed tag) well?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's basically a skip, so any sequence of {@link is handled correctly. It doesn't handle the unbraced @link ... or the extra space in { @link, neither of which are legal jsdoc, but could occur.

Copy link
Member Author

Choose a reason for hiding this comment

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

After thinking about it a bit more:

  1. unbraced @link is formatted like a top-level tag, so I think it's fine to treat it as one.
  2. skipping arbitrary whitespace between { and @link is a lot of work with the current tokeniser. We should switch to the normal tokeniser and then revisit this code.

Choose a reason for hiding this comment

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

So only tags named "link" has this special behavior ? Isn't this kind of arbitrary and unexpected behavior? Reminds me something I heard about an old windows version having a code like if(process.name==='the sims'){increaseSomething()}.

What's the reason for this? Is it for IDEs trying to support link in JavaScript projects ? Is it really important to hack the parser like this so one or two IDEs have an easy way of locating the parent tag description and position to insert a link ? They can still implement that (although it's harder/hacky/slow for them) without this hack in the compiler...

In which kind of projects are they trying to support @type ? Which reference syntax are these projects using , the usejsdocs.org one or TypeScript one?

In the later case, these projects are already breaking usejsdocs "standard" so they should not be using @link or {@link} but something else

In the first case, then I assume the motive for this hack is to support IDEs so they can easily/fast recreate the broken parent tag description and find the position to insert the anchor. If that's the case don't you think they should be responsible of solving their own problems? And if, as best case, they are just trying to solve the problem of jsdoc referencing once and for all, do you think this is the correct way of doing it?

If this is more or less right, then I'm worried that you didn't consider nor the compiler API user (for which BTW you introduced a breaking change), nor in the jsdoc writer for which TypeScript could provide typechecked references supporting refactors easily, and nor on typeScript developers since althoguh this change affected compiler performance minimally it shows performance place in this project priorities

I made an informal quick proposal #16498 (comment) and I would love to document in more detail discuss and implement it, but I would like first to know if such feature would be accepted ?

I think TypeScript now has the opportunity to define the definitely and single way of document JavaScript APIs as other languages have, and a feature like this would help a lot in that direction.

Don't take me wrong, I admire this project's contributors , but this just surprise me, sorry for the long comment, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

was The Sims part of the Windows spec? In this case @link is part of jsdoc that Typescript explicitly wants to opt out of. And it's the only non-top-level tag in jsdoc, which means that code to support it (or skip it) will look special.

@sandersn sandersn merged commit 99ebcd7 into master Jun 25, 2018
@sandersn sandersn deleted the jsdoc/link-tag branch June 25, 2018 22:27
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