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

More control delegated for external link resolvers #2066

Merged
merged 13 commits into from
Feb 24, 2023

Conversation

captain-torch
Copy link
Contributor

Adding external link resolvers was a great idea, but in fact, it is useless for {@link} tags which cannot be resolved (from documentation), because linkResolver passes into external resolvers only DeclarationReference, that will be invalid for links in formats that is not supported. So external resolver will know about fact that some link can't be resolved, but can't do anything
about it.

I offer an improvement:

  • Pass additional information (part: CommentDisplayPart and reflection: Reflection) to external resolver
  • Allow external resolver control both target and text of link
  • Add an exported ExternalResolveAttempt and ExternalResolveResult types as API for external resolver

@captain-torch captain-torch marked this pull request as draft October 3, 2022 03:36
@captain-torch captain-torch marked this pull request as ready for review October 3, 2022 05:49
Copy link
Collaborator

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of giving more control here... a few comments, and a question:

Why should code have access to the reflection containing the link when resolving a link to something outside of the documentation?

src/lib/converter/comments/linkResolver.ts Outdated Show resolved Hide resolved
Comment on lines 283 to 284
part?: CommentDisplayPart,
refl?: Reflection
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be non-optional, given your changes the array type above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was made optional because of usage of this function in LinkResolverPlugin (line 53). I didn't catch the purpose of using this function there, and for safety decided to change resolveExternalLink signature in order to avoid modifying of this code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is for links to types declared outside of the project, e.g.:

import { Bar } from "external-lib"
export function foo(bar: Bar) {}

src/lib/converter/converter.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Feb 10, 2023

and reflection: Reflection

There might be a legitimate use case for this, but I'm not seeing it. Why should code intended to resolve links to pages outside the documentation have this?

@captain-torch
Copy link
Contributor Author

In my case, we have few libraries, which have imports between them, but typedoc combines them all into one useful documentation (by another plugin). With current logic, typedoc resolves links to our own entities like it is external. But in fact, somewhere in reflections tree is a reflection that contains information about this entity, and I passes reflection, where link is located, to build relative links between these entities.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Feb 24, 2023

That shouldn't be necessary in an ideal world... but given how much time I'm spending here... probably not going to be fixed soon, and this isn't a huge wart. I guess I'm fine with it.

Also swap argument order so that the possibly undefined parameter is last.
@Gerrit0 Gerrit0 merged commit 020ebee into TypeStrong:master Feb 24, 2023
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Feb 24, 2023

Thanks! I'll get this released by Monday at the latest.

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

Successfully merging this pull request may close these issues.

2 participants