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

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

Open
HarelM opened this issue Jan 27, 2024 · 9 comments
Open

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

HarelM opened this issue Jan 27, 2024 · 9 comments
Assignees
Milestone

Comments

@HarelM
Copy link

HarelM commented Jan 27, 2024

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
Copy link
Author

HarelM commented Jan 27, 2024

cc: @Gerrit0

@timocov
Copy link
Owner

timocov commented Jan 27, 2024

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
Copy link
Author

HarelM commented Jan 27, 2024

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
Copy link
Owner

timocov commented Jan 27, 2024

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?

@timocov timocov added the Bug label Jan 27, 2024
@timocov timocov added this to the 9.3 milestone Jan 27, 2024
@timocov timocov self-assigned this Jan 27, 2024
@timocov timocov changed the title Using typedoc on bundler output tsdoc comments aren't renamed alogn with their nodes Jan 27, 2024
@Gerrit0
Copy link

Gerrit0 commented Jan 27, 2024

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
Copy link
Owner

timocov commented Jan 28, 2024

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
Copy link

Gerrit0 commented Jan 28, 2024

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

@timocov
Copy link
Owner

timocov commented Jan 28, 2024

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 timocov changed the title tsdoc comments aren't renamed alogn with their nodes jsdoc comments aren't renamed alogn with their nodes Jan 29, 2024
@timocov
Copy link
Owner

timocov commented Jan 29, 2024

@timocov timocov modified the milestones: 9.3, 10.0 Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants