Feature/link fixes and additions

[bradennapier]

For full context on this PR: #98238

The PR was rejected due to adding the concept of workspace:// and project:// to links, which would be nice to have, but that PR did fix some other issues with handling of links with todays features as well.

This PR removes the workspace and project protocol link handling and instead fixes the bugs mentioned around display the links as well as clicking the links themselves when using @link.

General Features:

• Fix for cases when using {@link https://www.google.com|Link Name} so it doesn't link to https://www.google.com|Link
• Allow file:// protocol to use relative linking. Today file://../file.ts would fail among other cases mentioned below.
• Allow using the jsdoc @link syntax of {@link https://www.google.com A name for the link}
• Fix handling of relative linking in hovers and popups where possible so they link relative to the file the type is defined in and not the file the person is viewing it from. See TSServer Completions - Context Needed TypeScript#38738 for issue which makes this impossible in some places.
• In the places that it is impossible to derive the location of the type and a link is encountered, it will not be rendered as a link in the hover. This occurs sometimes with completions (but not all the time). So it handles this intelligently.
• Intercept and rewrite markdown links if necessary (currently markdown links will not work in preview hover if they use file:// relative paths)
• Allow the use of fragments for global-level and preview links (currently broken in vscode upstream) (allowing linking to specific line/column via file.ts#3,1)

Examples

Below is an example that can be used to check the various cases that this resolves.

/**
 * - file:///Users/user/file.ts <- Absolute FileSystem
 * - file://./index.ts <- Relative File
 * - file://../index.ts <- Relative File in directory above itself
 * - [Relative to index via file://./index.ts]( file://./index.ts ) <- Relative File
 * - {@link file://./index.ts Relative to index via @link and file scheme}
 * - {@link file://./index.ts#3,2|Link to index.ts line 3, column 2}
 * - {@linkcode https://www.google.com Google Link as linkcode}
 * - https://www.google.com
 * - [Link To Google](https://www.google.com)
 */
export const links = {} 

Importantly, the links will properly resolve the relative paths when the hover/previewer is rendered from a path outside of the path links is defined within.

Extras

• Would really like to get project path support in here if possible (and workspace would be nice as well). Perhaps instead of a protocol we could go with the recommended method mentioned in the PR of using a magic string which is what is done by Flow team for handling these cases? Something like {@link file://<PROJECT_PATH>/... or something along these lines.
  • I could have that working in a matter of hours since it would just need to adapt what I had already done with the protocol paths.

Hoping we can discuss and merge @mjbvz

[mjbvz]

Some of the proposed sound good but I have concerns about much of how they are being implemented. Again, I think this should be discussed more on the TypeScript side before we add much more code to handle these cases to the TypeScript extension for VS Code

[mjbvz]

This adds an extra request every time we need to resolve a completion. I don't think the benefit this information is worth the cost. Instead, any information required should come from TS as part of the completionEntryDetails response

[bradennapier]

For completion details it probably isn't worth it too often. 99% of the time this will be useful is during the hover, which also has to do this. This is mainly due to the issue I rose over there microsoft/TypeScript#35524

It could definitely be removed here and left there to retain almost all the benefits of the PR with relative linking. The rendering will already expect that sometimes definition will be undefined and simply render the appropriate text in the case it sees a relative url.

So if {@link file://../test.ts#3 test.ts} would just render as test.ts and {@link file://../test.ts} as ../test.ts

[mjbvz]

This would also add an extra request for hovers, which I am also not a fan of since this is also a user facing operation and delays are noticeable. Again, I think any information should come from TS as part of the response

[mjbvz]

We also do not want to add complex link parsing logic to the VS Code extension as this would not benefit any other consumers of the TS service. Instead the parsing should likely happen on the TypeScript side, with VS Code simply transforming the returned data into a usable format

[bradennapier]

I am not sure this is really much more complex when all is said and done, but I understand the concern.

This is already parsing for {@link} so this is simply adding conditions to that same parsing which should not add much overhead unless it gets a match. Also, does this not only run on the previewer.ts context which would always get benefit from these changes? Any time rendering preview hovers with markdown this would be useful and relative linking should be a benefit to all files and content regardless if they are Typescript.

The only real thing this adds would be beneficial 100% of the time in that case. The parsing fixes [](file://./) linking and file://./ linking in the markdown text. Those are not Typescript-specific.

You should probably not support linking at all if it doesn't really work. At least just ignore relative urls as well as {@link} because:

1. @link will never work when cmd+clicking them if they have a label since it will try to open the wrong url but in the hover it will be correct, which means the behavior is not only inconsistent but broken.
   • Another option is to not support the | syntax at all for link and just have the {@link url this is the text} which this PR does make work.
   • Only supporting {@link https://www.google.com Google} syntax also resolves the issue brought up in contrib/links/links. Only the fixes to allow ../ style relative linking would be needed there then.
2. relative urls will only work in some case, but not most such as ../ or when rendered from another path in the project.
3. file:// urls also do not work currently when relative and using @link
4. file:// urls will never work when using markdown [test](file://./plugins.ts)
5. file:// urls will never work with fragments - which this does not really make more complex to support file://./plugins.ts#3,6

Probably should just remove relative link support completely so-as not to cause problems since at the moment it will fail almost 100% of the time.

[mjbvz]

The existing @link parsing was added as a quick solution that addresses the simplest case. {@link http://example.com} is what the majority @link uses I've seen look like.

Without more user feedback, I'm also not convinced there is enough demand for file://./`style link support. Such links just look wrong to me and I also don't know if other tooling would support these

[mjbvz]

This is VS Code's core link detection logic which is pretty language agnostic. I'm not sure this change belongs here (especially because some valid urls do include |)

Instead, we would likely want to handle this in the TypeScript extension

[bradennapier]

The "|" character is considered "unsafe" according to the RFC and would have to be encoded. I do believe this is the reasoning around this character being used to separate URL from the label with jsdoc. Although if it can be fixed on typescript level for the content in editor and hover then yes that would be ideal, not familiar how that would work.

https://www.ietf.org/rfc/rfc1738.txt

   Characters can be unsafe for a number of reasons.  The space
   character is unsafe because significant spaces may disappear and
   insignificant spaces may be introduced when URLs are transcribed or
   typeset or subjected to the treatment of word-processing programs.
   The characters "<" and ">" are unsafe because they are used as the
   delimiters around URLs in free text; the quote mark (""") is used to
   delimit URLs in some systems.  The character "#" is unsafe and should
   always be encoded because it is used in World Wide Web and in other
   systems to delimit a URL from a fragment/anchor identifier that might
   follow it.  The character "%" is unsafe because it is used for
   encodings of other characters.  Other characters are unsafe because
   gateways and other transport agents are known to sometimes modify
   such characters. These characters are "{", "}", "|", "\", "^", "~",
   "[", "]", and "`".

[mjbvz]

Please split this into its own issue then. It is not js/ts specific should likely be fixed here: https://github.com/mjbvz/vscode/blob/5bcfb8e897669f0069c73c69506d6371dd7ae3d8/src/vs/editor/common/modes/linkComputer.ts#L276

(Although again, I seem to recall some links using | in their query strings)

[mjbvz]

Closing this since we'd like more community feedback before investigating the additional types of links proposed

The TypeScript team has committed to basic JSDoc @see and @link tag support (microsoft/TypeScript#35524) so please also continue following that issue