timocov / dts-bundle-generator

A tool to generate a single bundle of dts with types tree-shaking
MIT License
770 stars 40 forks source link

jsdoc comments aren't renamed alogn with their nodes #295

Open HarelM opened 10 months ago

HarelM commented 10 months ago

It was suggested in one of the PRs I did, that I should try running typedoc on the output of dts-bundle-generator. This should save the complexity of adhering to both packages rules, as both do similar things when it comes to exporting things that are not part of the public API (typedoc has a plugin called typedoc-plugin-missing-exports and dts-bundle-generator has a flag called --export-referenced-types, which I believe are very similar).

So I tired it on my library and I got some warnings from typedoc. While some warning are related to missing export of some classes/types in the public API, others I did not know how to solve.

I'll try to explain the problem as best as I understand. When using dts-bundle-generator some classes that are colliding with global classes are being assign a temporary name, in my case Map (which is a map - as in map to show countries and geography) is colliding with the javascript Map<T, U> class. Also there's an Event class, they get Map$1 and Event$1 respectively. Due to history and a lot of users, I can't easily change their names. typedoc is trying to look at the tsdoc comments and create href links between classes documentation, for example to the map method map#render. When the class name is changed these links get broken and typedoc reports warnings, which I set to be errors so I won't have warnings creeping into the docs generation.

I can't think of an easy solution to the problem that does not involve disable this name collision feature, but there might be other ways :-)

Feel free to use my repo for experimentation: https://github.com/maplibre/maplibre-gl-js

Let me know if more info is needed.

HarelM commented 10 months ago

cc: @Gerrit0

timocov commented 10 months ago

typedoc is trying to look at the tsdoc comments and create href links between classes documentation, for example to the map method map#render.

So is the issue that the reference in tsdoc comment isn't being renamed when a node is renamed? Or there is something else?

HarelM commented 10 months ago

I'm not sure if a rename to the comment would be the right approach as you might see in the docs Map$1, which is not what I would like the people reading the docs to see. But if it solves the warning and does not appear in the docs, then sure, it's a valid solution.

timocov commented 10 months ago

I'm not sure if a rename to the comment would be the right approach as you might see in the docs Map$1, which is not what I would like the people reading the docs to see.

Well, its up to a docs renderer to decide whether to use an exported name in the docs or a local (referenced) one. But what I'm worried about right now is actual jsdoc values - when you read them you will see Map$1 and it will navigate you to a correct type but in your mind it might be confusing. I don't think it should be a big problem (at least the result is correct), but I don't see any other solution here, unfortunately. We can't just disable collisions resolver, even with Map example if you will use anywhere in your code/types JS type Map<K, V> then they will get merged and it will be either a compilation error (good, you can detect it) or they will silently get merged and this is very dangerous situation.

So can you confirm that the issue is related to jsdoc comments only? I.e. if you have something like this:

export interface Map {
}

/** See {@link Map} */
export type MyMap = Map;

then when Map gets renamed to Map$1 the comment remains the same i.e. /** See {@link Map} */ instead of /** See {@link Map$1} */, is that what you're experiencing right?

Gerrit0 commented 10 months ago

It's worth mentioning that links in comments are resolved by TS's language service if you use the local name, but not if you use an exported name.

/**
 * {@link OtherName.method} doesn't work
 * {@link Map.method} works
 */
export interface Map {
    method(): void
}

export { Map as OtherName }

If you're going to rewrite this, you might consider rewriting {@link Map.method} to {@link Map$1.method | Map.method} so the rendered link text will be the same as the exported name. (Unless the link tag already has text, in which that ought to be preserved.)

timocov commented 10 months ago

It's worth mentioning that links in comments are resolved by TS's language service if you use the local name, but not if you use an exported name.

Yeah my current plan is to fix jsdoc comments so they will refer to a correct identifier (i.e. Map$1 instead of Map<K, V>).

Thanks for pointing at | Map.method thing - I will consider adding this as well but it might be tricky because 1) node might be not exported (which is fine, just not add text) 2) node might be exported with multiple names (its unknown which one to pick to provide move "meaningful" value). Right now it feels like this can be controlled from the user side if we decide to keep comments as is. It is not a big problem if the value will be Map$1.method but it refers to a correct link so a user can navigate to it. Unless typdoc doesn't resolve track symbols and Map$1.method will not navigate to exported Map.method?

Gerrit0 commented 10 months ago

TypeDoc uses the compiler's resolution if available by default, though that's controlled by an option

timocov commented 10 months ago

It looks like the compiler doesn't provide an API to manipulate with attached jsdoc comments. I'll check later in discord chat/issue tracker if there anything we can do to update a comment accordingly.

timocov commented 9 months ago

https://github.com/microsoft/TypeScript/issues/57219