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

Verify expected Table of Contents (toc) implementation and behaviour #743

Open
techfg opened this issue Dec 21, 2024 · 19 comments
Open

Verify expected Table of Contents (toc) implementation and behaviour #743

techfg opened this issue Dec 21, 2024 · 19 comments
Labels

Comments

@techfg
Copy link

techfg commented Dec 21, 2024

What package is the bug related to?

typedoc-plugin-markdown

Describe the issue

When configured to use typedoc-plugin-remark and remark-toc, the generated markdown for Type Alias pages do not contain a Table of contents even when they contain at least two ## headings.

The reason appears to be that type aliases are not included in the kinds of reflections that are eligible for table of contents.

Not sure if this is a bug and it should be working or if this is by design and Type Aliases are intentionally bypassed. Given a type alias can have lots of properties, having a TOC can be very helpful. If this is intentionally by design, happy to change this to a feature request to allow for config option (or similar) to control the types that toc is automatically added to.

Repro: https://stackblitz.com/edit/vitejs-vite-wc5ogkjb?file=src%2Findex.ts%3AL10

npm run docs

Interface
image

Type Alias
image

TypeDoc configuration

    "typedoc": "^0.27.5",
    "typedoc-plugin-markdown": "^4.3.3",
    "typedoc-plugin-remark": "^1.2.1",

Expected behavior

Contents heading should be generated so that remark-toc has somewhere to put it :)

Happy to work on a PR for this once its determined the correct direction (if any) to go.

@tgreyuk
Copy link
Member

tgreyuk commented Dec 22, 2024

@techfg thanks.

When configured to use typedoc-plugin-remark and remark-toc, the generated markdown for Type Alias pages do not contain a Table of contents.

The initial intention was to add the toc heading as equivalent to where the html theme adds the "Index" section. The html theme only adds Index sections to Classes, Interfaces and Enums, I suppose with the rationale that these symbols always contain groups of Properties, Methods etc.

However your use case does illustrate how it would also be useful for Type Aliases when there is a properties list. However a toc might not always be desirable.

We could potentially end up with undesirable headings for simple Types:

Screenshot 2024-12-22 at 21 01 02

I am not entirely sure what the best solution to this is? It might be as simple as adding the toc heading when there is at least one other than the "Defined In" heading?

Happy to work on a PR for this once its determined the correct direction (if any) to go.

Would be very happy to have some more active contributors. I appreciate that the code is probably not that intuitive or well documented.

I have actually created a new branch for this bit of work named feature/remark-toc. If you were to look into it I would branch off that.

This contains a few tweaks:

  • the logic to handle adding the heading is now a local remark plugin itself
  • have removed the heading count restrictions

@techfg
Copy link
Author

techfg commented Dec 23, 2024

Thanks @tgreyuk!

You've made me re-think the original intent of this issue and (probably) change my perspective lol

Firstly, I was always thinking of the toc as a replacement for the toc in the html version, not as the replacement for index so great to know what the original intent was. One that I think makes a lot of sense and I think trying to "mimick/replace" the index equivalent in the html version is better than trying to replace the toc in html version for reasons I'll describe below.

The other thing that you made me realize, after doing some more testing, is that the html version does not have an index section for Type Aliases, even when properties are listed vs simple types. I do think this is a shortcoming of the html version and a use-case where they should be emitted as it could be very helpful (and what led to this issue being created). However, it has to be smart about when to add index section and when not to, as you mention.

In short, I think the first thing to decide for this plugin is if the intent is to replace the html version of the toc or index because that decision does change things to a certain degree although minimal. For example, the html version of the toc does not emit @groupDescription while the index does.

The other thing to note is the Defined In section. Currently, markdown plugin emits as h2 but I think it should be considered to just emit this as text block without a heading. The reason being that these h2 blocks get added to the current toc which is not consistent with the html version. This assumes of course that the toc remains to be built by remark-toc instead of potentially considering building the index section directly via the markdown plugin (I think this is the way it used to be done??).

Ok, getting lengthy here so I'll wrap up for now 😄 Beyond the above comments:

  1. As we've discussed elsewhere, I think markdown plugin should mimick html version as closely as possible except for where it does not make sense because of markdown vs. html nuances. Given that, html version does not currently emit an index section for type alises, even when expanded with properties, so likely markdown shouldn't either. This is the way markdown plugin currently works, and what this issue was filed against, but in re-thinking based on your input, maybe it should stay as it is. I do think that an index of props for type alises, when properties exist, would be helpful and do think that html version should include but I think the priority should be consistency with html so possibly leave as it is now in markdown or add an option to add it when option is enabled.
  2. html version emits an index regardless of how many properties exist and other "sections" in the doc so I think removing the heading count restrictions, like you did in the PR, is the right path forward.
  3. html version does not emit a toc or index for interfaces, etc. when there are no props/methods/etc. so where markdown does emit an index/toc, it should condition around making sure that there is one (or more) of the possible other sections (e.g., props, methods, etc.) to qualify (aka. instead of using heading count).

FWIW, my recommendation:

  1. markdown should attempt to replace the equivalent of the html toc instead of the index. While the html difference between the two is subtle, a couple of reasons:
    1. index does contain more information (e.g., @groupDescription) than toc` in html version
    2. from what I can tell, they always both exist or don't exist, haven't found a situation where toc exists and index doesn't or vice-versa
    3. markdown relies on remark-toc for the toc, all its doing is putting in the relevant block placeholder so it really is a vanilla toc. If the intent is to replace index, possibly remark-toc shouldn't be relied on and instead, markdown generates the index since it will have access to the other type/etc. information necessary to make it fully intelligent output.
  2. Defined in should just be block text, doesn't require a heading
  3. By default, type aliases do not have a toc/index so that they mirror html version. It would be nice and helpful to have this when properties exist so possibly consider a way to opt-in or if the default is to include, a way to opt-out (e.g., support a @noTableOfContents tag or something similar).

Happy to contribute to the PR you started once you have a chance to think through all this and let me know which direction you ultimately want to go.

Thank you again!

Edit: Submitted TypeStrong/typedoc#2817 to add support for toc & index in default html theme for type alias when it has properties.

@tgreyuk
Copy link
Member

tgreyuk commented Dec 23, 2024

@techfg thanks

Ok, getting lengthy here so I'll wrap up for now 😄 Beyond the above comments:

Agree on points 1,2,3 here.

index does contain more information (e.g., @groupDescription) than toc` in html version

I notice that group descriptions are missing from module index pages - i will add that. (this is not an inline toc so a slightly different use-case).

A)

Screenshot 2024-12-23 at 13 45 36 Screenshot 2024-12-23 at 13 45 42

For inline "indexes" or inline tocs i notice that html theme adds the group description only to the index and not with the group content. I am not sure that this pattern is better, or at least not compatible with the markdown theme where many users reply on auto generated toc sidebars for inline linking.

B)

Screenshot 2024-12-23 at 13 49 04 Screenshot 2024-12-23 at 13 49 17

markdown relies on remark-toc for the toc, all its doing is putting in the relevant block placeholder so it really is a vanilla toc. If the intent is to replace index, possibly remark-toc shouldn't be relied on and instead, markdown generates the index since it will have access to the other type/etc. information necessary to make it fully intelligent output.

Previous versions of the plugin did actually provide the inline toc, but it seemed most people switched it off in favour of auto sidebars generated from content headings as per users markdown environment. It was also quite challenging to manage manually with additional context required of if things if items are rendered within tables etc. The other benefit it that the toc includes any tagged content headings created by document comments such as @example, @see, @remark etc etc. remark-toc also comes with some additional options to control output. You have me second guessing myself now though :)

Defined in should just be block text, doesn't require a heading

Potentially not - this is actually something that has been grappled with in the past. The rationale is that headings provide a better semantic hierarchy to the pages.

For example what I don't quite like about this example is that the "Defined In" text is grouped within the "Type declaration" heading:

Screenshot 2024-12-23 at 14 17 50

Having it as a separate heading provides a better integrity of heading heirarchy:

Screenshot 2024-12-23 at 14 21 45

Another example here from the html theme is that "Returns" and "Defined in" are both placed under the "Parameters" heading where they are actually distinctive content elements in their own right:

Screenshot 2024-12-23 at 14 24 08

Markdown equivalent is probably semantically better:

Screenshot 2024-12-23 at 14 26 47

Anyway that is the rationale, could potentially move defined in up to the top of the symbol like html theme does for interfaces (it is not completely consistent the where think link is placed in html theme either).

Edit: Having reviewed this I think we can definitely make work as inline text by moving the position of the link (see linked issue).

Happy to contribute to the PR you started once you have a chance to think through all this and let me know which direction you ultimately want to go.

I'll add what have done so far (basically remove heading restrictions) but keep other logic and monitor the response from TypeDoc on the linked issue.

@techfg
Copy link
Author

techfg commented Dec 23, 2024

For inline "indexes" or inline tocs i notice that html theme adds the group description only to the index and not with the group content. I am not sure that this pattern is better, or at least not compatible with the markdown theme where many users reply on auto generated toc sidebars for inline linking.

Yeah, I noticed this as well. I actually prefer the way markdown does this over what html does, putting the descriptions within the "group content" and not the "index" if it's going to be one or the other. There is something to be said for putting in both, but at same time, that might be overkill. I can understand why html does it in the index as it's a quick reference type of concept but I can also see scenarios where people don't look at the index, go straight to the group content and then miss out on the extra contextual information. My $0.02 is markdown maintain its current behavior.

Previous versions of the plugin did actually provide the inline toc, but it seemed most people switched it off in favour of auto sidebars generated from content headings as per users markdown environment. It was also quite challenging to manage manually with additional context required of if things if items are rendered within tables etc. The other benefit it that the toc includes any tagged content headings created by document comments such as @example, @see, @remark etc etc. remark-toc also comes with some additional options to control output. You have me second guessing myself now though :)

Not sure I 100% follow you here. Understood what markdown used to do (build it itself) vs. what it now does (defer to remark-toc). Are you saying remark-toc would be able to handle things like @example, @see, etc.? The point I was trying to make was that the current behavior to rely on remark-toc seems to be more of a true table of contexts (no extra contextual information) vs. an index (some extra contextual information) but if remark-toc can do some extra context then that changes things. Ultimately, I think it comes down to what is the Contents area and is it intended to be a simple table of contents (identical to html toc) or something more robust (similar/identical to html index).

Another example here from the html theme is that "Returns" and "Defined in" are both placed under the "Parameters" heading where they are actually distinctive content elements in their own right

Possibly I'm misunderstanding what you're saying but in html they are all siblings
image

Edit: Having reviewed this I think we can definitely make work as inline text by moving the position of the link (see linked issue).

Agreed!!

I'll add what have done so far (basically remove heading restrictions) but keep other logic and monitor the response from TypeDoc on the linked issue.

Didn't see feature/remark-toc updated yet, will take a look once you push. Thanks!

@tgreyuk
Copy link
Member

tgreyuk commented Dec 23, 2024

Not sure I 100% follow you here. Understood what markdown used to do (build it itself) vs. what it now does (defer to remark-toc).

Apologies, I guess I was just outlining the reason for opting for the remark-toc route (simple toc following page headings) over bespoke solution more in line with html theme. When block tags are used in doc comments they are converted into page headings so the remark plugins adds these to the toc whereas the html theme index will ignore them. But the two approaches are different and the Contents area on markdown theme intention is currently to be a simple heading based opt-in toc.

Possibly I'm misunderstanding what you're saying but in html they are all siblings

Ah! I provided an invalid example so stand corrected. Html has the benefit of using block elements to separate the content and in this example the sources block is in an <aside> element and is therefore a sibling and not a child of the h4. So this is semantically valid, but following the same pattern in markdown would not work because it can't utilize block level elements in this way. Hope that provides 'some' clarity, but anyway, as discussed we are moving the "Defined In" heading so is now a moot point.

Didn't see feature/remark-toc updated yet,

[email protected] removes the heading restrictions as discussed and has been merged to the main branch.

@techfg
Copy link
Author

techfg commented Dec 23, 2024

[email protected] removes the heading restrictions as discussed and has been merged to the main branch.

Ah, misunderstood, thought you were going to implement the Defined in changes in same PR but makes sense to keep separate assuming you're still thinking of doing that?

@tgreyuk
Copy link
Member

tgreyuk commented Dec 23, 2024

Ah, misunderstood, thought you were going to implement the Defined in changes in same PR but makes sense to keep separate assuming you're still thinking of doing that?

Yes defo still going to do, but the main plugin requires a lot more due diligence around the release process. Can track the progress on that with #746.

@techfg
Copy link
Author

techfg commented Dec 23, 2024

Cool, makes sense!

Given the discussion, for anyone that might stumble upon this in the future and try to reverse-engineer all the discussion, here's where I think things have landed:

  1. Including toc & index on type alias pages (the original purpose of this issue) - For now, going to maintain current functionality and not do this to remain consistent with typedoc html default theme. Will monitor include table of contents & index on type aliases that contain properties TypeStrong/typedoc#2817 to see if/when the default theme implements and if so, adjust accordingly.
  2. Eliminate heading count restrictions - Originating from support controlling when a table content is generated #744, eliminate heading count restrictions and always emit table of contents for eligible reflection kinds as long as there is at least one prop/method/etc.
    • Question: npm is failing to install from main so I can't verify but from reviewing code, it looks like a toc could get included even if there are no props/methods/etc.??
  3. Move Defined in location which is tracked via Inline Defined in headings #746

I think that sums it up unless I'm missing something?

EDIT Tested with v1.2.2 of typedoc-plugin-remark and no TOC is emitted when there are no props/methods/etc. which would be expected behavior.

@tgreyuk tgreyuk changed the title Table of Contents (toc) not generated for type aliases when using typedoc-plugin-remark & remark-toc Table of Contents (toc) verify behaviour when using typedoc-plugin-remark & remark-toc Dec 30, 2024
@tgreyuk tgreyuk added question and removed bug Issue raised as a bug. labels Jan 1, 2025
@tgreyuk tgreyuk changed the title Table of Contents (toc) verify behaviour when using typedoc-plugin-remark & remark-toc Verify expected Table of Contents (toc) implementation and behaviour Jan 1, 2025
@tgreyuk
Copy link
Member

tgreyuk commented Jan 1, 2025

@techfg I will leave this one open and have a think about it. On reflection it might be better to move inline toc functionality into the core package rather than rely on remark-toc.

@tgreyuk
Copy link
Member

tgreyuk commented Jan 4, 2025

@techfg having looked at this I think we can cover this with a new option showInlineIndexes.

I think the name showInlineIndexes is clear and perhaps better than referring to the section as a toc.

Currently this will replicate the behaviour of the default html theme and include an inline index section for Classes, Interfaces and Enums. We could expand to variables and types that have type declarations if required though.

Do let me know if you have a particular preference of how this should behave.

I do think this is more preferable than pulling in remark just for this. (the remark plugin is still very useful but perhaps not for this).

@techfg
Copy link
Author

techfg commented Jan 4, 2025

@tgreyuk -

I do think there are some advantages to rolling the index/toc internally vs. deferring to remark, that said, I think the current approach does meet the need of a toc with minimal configuration.

The benefit of changing the approach to internal would be ability to provide additional information (e.g., group descriptions, etc.).

It would be nice to support the ability to generate for types/variables but given html does not do this (and likely won't for the forseeable future), if supported, it should likely require an additional flag. Possibly --showInlineIndex is false | true | "all" where false is never, true is default types (classes, interfaces, enums) and all is the default types + types & variables.

I think the first question is whether or not you are thinking --showInlineIndex would mirror the behavior of the html toc (only headings) or would it mirror the html index behavior (heading, description and then a subsection for props/methods/etc.)?

@tgreyuk
Copy link
Member

tgreyuk commented Jan 4, 2025

I think the first question is whether or not you are thinking --showInlineIndex would mirror the behavior of the html toc (only headings) or would it mirror the html index behavior (heading, description and then a subsection for props/methods/etc.)?

The current thinking it would mirror the heading structure of html theme without the group description, with the reason being the group descriptions are already included in the main content.

@techfg
Copy link
Author

techfg commented Jan 4, 2025

Ok, assumed that was the case and makes sense, just wanted to be sure.

Given that, I think using the index terminology may be misleading since, without additional information included within it, I think it's a direct (or at least more closely correlated) comparison to the html toc rather than the html index unless I'm misunderstanding what you are thinking?

@tgreyuk
Copy link
Member

tgreyuk commented Jan 4, 2025

Yes i think you are right.

In which case maybe an option name of showInlineTocs or simply showTocs is better. And in that case i guess "Contents" is a better title (over Index)?

@techfg
Copy link
Author

techfg commented Jan 4, 2025

I think includeTOC (or includeToc) may be better than show since "show" implies that its hidden by default vs. not there at all?

Whatever the name, if you are thinking of supporting types & variables, I think false | true | "all" or something similar makes sense with the default of only emitting in the same places that html does.

Regarding title, html uses "On This Page" but that seems inappropriate for markdown - I think either "Contents" or "Table of Contents" would be preferrable.

With all this said, I guess the question now is why change from remark-toc - the only advantage I'm seeing is the ability to provide a TOC for types/variables, am I missing other advantages beyond that and no longer requiring users to include remark-toc?

@tgreyuk
Copy link
Member

tgreyuk commented Jan 4, 2025

With all this said, I guess the question now is why change from remark-toc - the only advantage I'm seeing is the ability to provide a TOC for types/variables, am I missing other advantages beyond that and no longer requiring users to include remark-toc?

Having talked this through I am no longer sure there is much advantage after all :).

If sticking with remark-toc I guess the question is are we happy to close this issue as per #743 (comment) ?

@techfg
Copy link
Author

techfg commented Jan 4, 2025

If sticking with remark-toc I guess the question is are we happy to close this issue as per #743 (comment) ?

I think the current remark-toc approach has feature parity with html toc and therefore yes, I think this issue can be closed and always revisited if needed.

That said, the only additional thing I would add is that adding toc to types/variables could likely be easily supported using the remark-toc approach if types/variables were included in the filter for when to emit Contents. I do think there is value in having a toc for these types and I think the html version would offer this if it didn't require a complete refactor of the approach it currently uses. With that said, I think the only reason to keep this open would be to consider a new option (e.g., --allToc or similar) that would force a TOC on all pages regardless of type. Haven't though this through completely but think it would work although it's not necessary as mentioned above about current feature parity with html toc 😄

@tgreyuk
Copy link
Member

tgreyuk commented Jan 4, 2025

That said, the only additional thing I would add is that adding toc to types/variables could likely be easily supported using the remark-toc approach if types/variables were included in the filter for when to emit Contents

ok sounds good. I will probably bounce some ideas over regarding how to handle this via remark-plugin. May aswell keep the discussion going on this issue.

@techfg
Copy link
Author

techfg commented Jan 4, 2025

ok sounds good. I will probably bounce some ideas over regarding how to handle this via remark-plugin. May aswell keep the discussion going on this issue.

Sounds good but just to re-emphasize, I think the current approach which does not include types/variables does match html toc and therefore I don't think it's a must have, just a nice to have 😄

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

No branches or pull requests

2 participants