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

hyperlink not generated for {@link type#property} #732

Closed
techfg opened this issue Dec 7, 2024 · 16 comments
Closed

hyperlink not generated for {@link type#property} #732

techfg opened this issue Dec 7, 2024 · 16 comments
Labels
bug Issue raised as a bug.

Comments

@techfg
Copy link

techfg commented Dec 7, 2024

What package is the bug related to?

typedoc-plugin-markdown

Describe the issue

When using @link to reference a type property, the generated markdown is not hyperlinked.

Repro: https://stackblitz.com/edit/vitejs-vite-rq6ycbce

export type Foo = { name: string };

/**
 * Foo Handler that takes {@link Foo} and uses {@link Foo#name name}
 */
export const handleFoo = (a: Foo) => {};

Steps to Reproduce

  1. Open repro
  2. npm run docs:md
  3. View docs-md/functions/handleFoo.md

Actual Result
Generated markdown does not hyperlink the name property:

image

# Function: handleFoo()

> **handleFoo**(`a`): `void`

Foo Handler that takes [Foo](../type-aliases/Foo.md) and uses name

Additional Information

  1. The issue does not occur when generating html files (npm run docs:html)
  2. The type Foo itself does get hyperlinked
  3. I tried applying useHTMLAnchors=true but this did not result in name being hyperlinked either. In fact, there didn't seem to be any anchor elements generated at all - possibly I'm misunderstanding useHTMLAnchors or this may be a separate bug/issue?

TypeDoc configuration

typedoc: 0.27.3
typedoc-plugin-markdown: 4.3.1
typescript: 5.7.2

Expected behavior

The property name should be hyperlinked.

@techfg techfg added the bug Issue raised as a bug. label Dec 7, 2024
@tgreyuk
Copy link
Member

tgreyuk commented Dec 8, 2024

Thanks - links on props are now resolved in [email protected]. You would only need to set useHTMLAnchors=true when the property is included inside a table (without associated heading anchor).

@tgreyuk tgreyuk added the fixed Fix implemented in latest version. label Dec 8, 2024
@techfg
Copy link
Author

techfg commented Dec 8, 2024

Thanks for the quick fix @tgreyuk and appreciate the clarification on usage of useHTMLAnchors!

Types are looking good with this fix but there is a strange issue with interfaces.

Repro: https://stackblitz.com/edit/vitejs-vite-zdmljw8o

When using typeDeclarationFormat='table' a link is generated for interface#property but it does not contain the property fragment in the url. This does not happen when typeDeclarationFormat is set to list or htmlTable. When adding useHTMLAnchors, the problem does not occur but since I don't believe typeDeclarationFormat has anything to do with interfaces, I would expect the interface link to generate correctly without requiring useHTMLAnchors, correct?

  • Using list or htmlTable (use npm run docs:md) - Expected
    image

  • Using table (use npm run docs:md2) - Not Expected
    image

  • Using table and useHTMLAnchors (use npm run docs:md3) - Expected
    image

@tgreyuk
Copy link
Member

tgreyuk commented Dec 8, 2024

but since I don't believe typeDeclarationFormat has anything to do with interfaces, I would expect the interface link to generate correctly without requiring useHTMLAnchors, correct?

You are absolutely correct, this isn't quite correct. The idea is that when an anchor cannot be resolved it fallsback to the nearest available anchor, but like you say the interface anchors should not be effected by the options you have set. Also foo.md#__type is not a correct fallback.

Thanks again - ill be fixed in next patch.

@tgreyuk
Copy link
Member

tgreyuk commented Dec 19, 2024

[email protected] includes fixes as discussed.

@techfg
Copy link
Author

techfg commented Dec 19, 2024

Thanks @tgreyuk!

Just tested with 4.3.3 & latest typedoc 0.27.5 and things are improved, however one issue remains although I'm not 100% sure if it's expected or unexpected.

When running docs:md2, the following is output - note that the fragment to #name is missing:
image

When running docs:md3 which includes --includeHTMLAnchors the expected output is generated:
image

Would you expect docs:md2 to include the fragment for #name or would this only be emitted when --useHTMLAnchors is specified?

Updated Repro: https://stackblitz.com/edit/vitejs-vite-hbrlg3nz?file=docs-md2%2Ffunctions%2FhandleFoo.md

@tgreyuk
Copy link
Member

tgreyuk commented Dec 19, 2024

Thanks @techfg,

Would you expect docs:md2 to include the fragment for #name or would this only be emitted when --useHTMLAnchors is specified?

This assumption is correct currently - the reason being that when properties are rendered in a table there is no anchor to link to in the page.

However I appreciate that this is not very intuitive so should be addressed. It also means that anchors will get added everywhere (including next to markdown headings) where they are not needed.

I think the most obvious thing to do would be to automatically add html anchors to relevant table cells in this use-case, or make "useHTMLAnchors" a bit smarter. I think for simplicity adding anchors automatically in this scenario makes sense.

The initial rationale for not adding html automatically was that not all parsers support html, but as GFM supports html and markdown tables do not render without GFM, this is not a valid consideration.

@techfg
Copy link
Author

techfg commented Dec 19, 2024

Thanks for providing the context/background.

The initial rationale for not adding html automatically was that not all parsers support html, but as GFM supports html and > markdown tables do not render without GFM, this is not a valid consideration.

Possibly I'm misunderstanding what you're staying here must markdown tables are supported beyond GFM I believe. With the recently added defaultRemarkPlugins remark-gfm may not even be used

If starting over, I think generating anchors everywhere with an option to restrict may be best. However, in order to not introduce a potential breaking change and to provide backwards compat, possibly extending --useHTMLAnchors to have a mode instead of just a boolean that would drive which anchors are emitted makes sense? Alternatively, a separate option that allows refined control over which leaving --useHTMLAnchors as it is. Not ideal to introduce a new option but thinking of backwards compat if anything were to change in the way the final generated output made.

I think if you do add anchors to tables by default for this use-case, there may need to be a way to disable them for certain scenarios/use-cases.

All that said, I'm not even going to pretend to understand all the complexities of typedoc & typedoc-plugin-markdown and may be misunderstanding your comment to begin with so take the above FWIW 😄

Thanks again!

@tgreyuk
Copy link
Member

tgreyuk commented Dec 19, 2024

@techfg thanks for comments.

Possibly I'm misunderstanding what you're staying here must markdown tables are supported beyond GFM I believe.

Yes you are correct, markdown tables are indeed supported beyond GFM (which was an early adopter). Its really a bit of a mute point as most users will be consuming markdown beyond CommonMark (which does not include markdown tables as part of its spec).

With the recently added remark-gfm may not even be used

This is relevant only when performing additional parsing with Remark using typedoc-plugin-remark. Remark converts a Markdown string to an AST and back to Markdown, supporting only the CommonMark specification by default.

However, in order to not introduce a potential breaking change and to provide backwards compat, possibly extending --useHTMLAnchors to have a mode instead of just a boolean that would drive which anchors are emitted makes sense?

For now i think we could preserve the "useHTMLAnchors" as a singular option which accepts a mode (but will also allow boolean value for backward compatibility).

  • "always" - add anchors everywhere - (same as true)
  • "never" - html anchors are never used (same as false)
  • "auto" - based on the context or configuration (ie add to table cells when relevant)

Do you think this makes sense?

@techfg
Copy link
Author

techfg commented Dec 20, 2024

Just to make sure I'm understanding your idea correctly:

  1. Change useHTMLAnchors from boolean to boolean | 'always' | 'never' | 'auto'
  2. Default behavior of the plugin remains the same, nothing changes in the emitted markdown from what is currently there (aka. useHTMLAnchors defaults to false/never)
  3. Specifying --useHTMLAnchors or --useHTMLAnchors always would change the behavior of the current equivalent (--useHTMLAnchors) to add anchors everywhere instead of just table cells
  4. auto can be used (e.g., --useHTMLAnchors auto) to control where/how anchors are emitted

Am I understanding correctly?

Also, for my education, what other scenarios beyond table cells would auto potentially apply to?

@tgreyuk
Copy link
Member

tgreyuk commented Dec 20, 2024

Am I understanding correctly?

  1. Correct
  2. Correct
  3. Equivalent to current useHTMLAnchors=true which will add anchors to any symbols whether it be a heading or table cell.
  4. The idea is that "auto" would only add anchors where they do not exist by default - ie table cells.

Also, for my education, what other scenarios beyond table cells would auto potentially apply to?

Anchors would only ever be applied to next to headings or inside a table cells. The initial problem this option was trying to solve is that some parsers do not automatically assign header ids to html. It was then extended to also include anchoring to properties in table cells.

Perhaps simply an option with flags might be better (or an additional option that defaults flags to true).

useHtmlAnchors: {
  headings: false,
  tables: false
}

Hope this makes a bit more sense :)

@techfg
Copy link
Author

techfg commented Dec 20, 2024

Thanks! Just one clarification....the current behavior of --useHTMLAnchors will add to both headers & table cells, correct?

Assuming yes to above, then always (aka true) is appropriate naming and I think you're approach makes a lot of sense as it maintains full backwards compat as a starting point.

From there, I do agree that having flags nested instead of just auto likely would be better since it will provide you more flexibility for future and also allow users to "self-configure" rather than rely completely on the plugin to "guess" (even if its a very educated one) on what "should" happen.

In short, always would put them everywhere the plugin supports (currently headers & table cells but could be more locations in future), while { headings: boolean, tables: boolean } provides the user control and both the plugin (aka. you 😄) and the user flexibility moving forward.

@tgreyuk
Copy link
Member

tgreyuk commented Dec 20, 2024

Thanks! Just one clarification....the current behavior of --useHTMLAnchors will add to both headers & table cells, correct?

Yes this is correct.

I will take this away - i think we are on the same understanding now - just need to decide on the exact option configuration!

@techfg
Copy link
Author

techfg commented Dec 20, 2024

Sounds great and thanks for the patience in answering all the questions, just wanted to make sure I had a full and accurate understanding prior to providing my thoughts :)

Appreciate all your work, thank you!

@tgreyuk
Copy link
Member

tgreyuk commented Dec 20, 2024

No problem at all - thanks for the discussion.

@tgreyuk tgreyuk removed the fixed Fix implemented in latest version. label Dec 30, 2024
@tgreyuk
Copy link
Member

tgreyuk commented Dec 30, 2024

@techfg .

Ok - after some deliberation on this I decided to implement what I think is the most simplistic and deterministic approach.

As per changelog entry:

Anchor IDs are now applied to linkable symbols within table rows by default. Previously, the useHTMLAnchors option was required, but since there is no alternative way to link to these items, this behaviour is now the default.

I don't consider this a breaking change and I think is acceptable to release this change under a minor version.

I will consider implementing the ability to turn this off if somebody has a good enough reason to request not to have it.

"useHTMLAnchors" now only applies to page headings for platforms that dont provide automatic header ids.

Hopefully this makes sense as I dont want to needlessly over complicate the options.

Available in [email protected]

@techfg
Copy link
Author

techfg commented Dec 31, 2024

Looks good in v4.4.0 per expected behavior in changelog, thank you @tgreyuk!

@tgreyuk tgreyuk closed this as completed Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue raised as a bug.
Projects
None yet
Development

No branches or pull requests

2 participants