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.

Sign up for GitHub

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
+45 −1
Merged

Treat link tag as comment #25206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a98f1e
First attempt at parsing. Doesn't work
sandersn Jun 25, 2018
14ebf4f
Parsing sort of works
sandersn Jun 25, 2018
f3afc99
Parse link tag as comment
sandersn Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6574,6 +6574,16 @@ namespace ts {
indent += whitespace.length;
}
break;
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.

Copy link

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.

nextJSDocToken();
pushComment(scanner.getTokenText());
nextJSDocToken();
}
pushComment(scanner.getTokenText());
break;
case SyntaxKind.AsteriskToken:
if (state === JSDocState.BeginningOfLine) {
// leading asterisks start recording on the *next* (non-whitespace) token
Expand Down
6 changes: 6 additions & 0 deletions src/testRunner/unittests/jsDocParsing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,12 @@ namespace ts {
* @param {object} o Doc doc
* @param {string} o.f Doc for f
*/`);
parsesCorrectly("",
`/**
* {@link first link}
* Inside {@link link text} thing
* @see {@link second link text} and {@link Foo|a foo} as well.
*/`);
});
});
describe("getFirstToken", () => {
Expand Down
28 changes: 28 additions & 0 deletions tests/baselines/reference/JSDocParsing/DocComments.parsesCorrectly..json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"kind": "JSDocComment",
"pos": 0,
"end": 127,
"tags": {
"0": {
"kind": "JSDocTag",
"pos": 63,
"end": 68,
"atToken": {
"kind": "AtToken",
"pos": 63,
"end": 64
},
"tagName": {
"kind": "Identifier",
"pos": 64,
"end": 67,
"escapedText": "see"
},
"comment": "{@link second link text} and {@link Foo|a foo} as well."
},
"length": 1,
"pos": 63,
"end": 68
},
"comment": "{@link first link}\nInside {@link link text} thing"
}
2 changes: 1 addition & 1 deletion tests/cases/user/TypeScript-Node-Starter/TypeScript-Node-Starter
Open in desktop
Submodule TypeScript-Node-Starter updated 49 files
+11 −2 .env.example
+7 −1 .gitignore
+1 −0 .vscode/extensions.json
+3 −15 .vscode/launch.json
+8 −2 .vscode/settings.json
+190 −75 README.md
+0 −5 copyStaticAssets.js
+5 −0 copyStaticAssets.ts
+18 −0 jest.config.js
+1,722 −1,346 package-lock.json
+51 −59 package.json
+22 −19 src/app.ts
+5 −5 src/config/passport.ts
+3 −3 src/controllers/api.ts
+1 −1 src/controllers/contact.ts
+6 −6 src/controllers/user.ts
+9 −6 src/models/User.ts
+1 −1 src/public/css/lib/bootstrap/_button-groups.scss
+1 −1 src/public/css/lib/bootstrap/_forms.scss
+1 −1 src/public/css/lib/bootstrap/_input-groups.scss
+1 −1 src/public/css/lib/bootstrap/_panels.scss
+1 −1 src/public/css/lib/bootstrap/_scaffolding.scss
+2 −2 src/public/css/lib/bootstrap/_theme.scss
+1 −1 src/public/css/lib/bootstrap/_variables.scss
+2 −2 src/public/css/lib/bootstrap/bootstrap.scss
+3 −3 src/public/css/lib/bootstrap/mixins/_tab-focus.scss
+ − src/public/fonts/glyphicons-halflings-regular.eot
+288 −0 src/public/fonts/glyphicons-halflings-regular.svg
+ − src/public/fonts/glyphicons-halflings-regular.ttf
+ − src/public/fonts/glyphicons-halflings-regular.woff
+ − src/public/fonts/glyphicons-halflings-regular.woff2
+4 −4 src/public/js/lib/jquery-3.1.1.min.js
+8 −4 src/server.ts
+0 −2 src/types/lusca.d.ts
+0 −48 src/types/passport-local.d.ts
+17 −0 src/util/logger.ts
+26 −0 src/util/secrets.ts
+2 −2 test/api.test.ts
+2 −2 test/app.test.ts
+20 −2 test/contact.test.ts
+2 −2 test/home.test.ts
+20 −2 test/user.test.ts
+1 −0 tsconfig.json
+1 −1 views/account/profile.pug
+1 −1 views/api/facebook.pug
+1 −1 views/api/index.pug
+8 −9 views/layout.pug
+2 −3 views/partials/footer.pug
+2 −2 views/partials/header.pug