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

Code links to external dependencies are broken #71

Open
B4nan opened this issue Aug 21, 2022 · 3 comments
Open

Code links to external dependencies are broken #71

B4nan opened this issue Aug 21, 2022 · 3 comments

Comments

@B4nan
Copy link
Contributor

B4nan commented Aug 21, 2022

With excludeExternals: false we can get external symbols rendered in the docs. The code links (link to the code definition) are broken for such.

Example: https://crawlee.dev/api/next/core#log
Link goes to: https://undefined/apify/crawlee/blob/master/node_modules/@apify/log/index.d.ts#L8

We should probably not have the link at all, I am not sure there is a universal way to know where the actual symbol is defined (e.g. this also happens with internal node API, when extending things like EventEmitter here).

@jan-molak
Copy link
Contributor

jan-molak commented Aug 21, 2022

@B4nan - I see value in linking to external docs, but like you say it might be tricky to find a general way to determine the link.

ESDoc had an interesting way of solving this problem; What you'd do is you'd create a list of external definitions like this one for EcmaScript or this one for Web API. In there, you'd add types and the links they should lead to. For example:

/**
 * @external {DocumentFragment} https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment
 */

/**
 * @external {Element} https://developer.mozilla.org/en-US/docs/Web/API/Element
 */

/**
 * @external {Event} https://developer.mozilla.org/en-US/docs/Web/API/Event
 */

Next, as ESDoc parsed the source code, it would look up any types it couldn't find in your code base in those external definition files.

I also came up with an idea to use namespaces in those definitions to avoid collisions between different external modules providing types with the same name. Serenity/JS integrates with several Web automation tools like Selenium, Playwright or WebdriverIO; they all have some flavour of a WebElement or Browser, so conflicts are unavoidable.

A type definition with a namespace would look something like in the example below, so <package name>~<type name>:

/**
 * @external {@wdio/types~Browser} https://github.com/webdriverio/webdriverio/blob/main/packages/webdriverio/src/types.ts
 */

/**
 * @external {@wdio/types~Element} https://github.com/webdriverio/webdriverio/blob/main/packages/webdriverio/src/types.ts
 */

So what I'm thinking is that while links to external types could be disabled altogether, perhaps some form of a static dictionary could work here too? 🤔

Alternatively, if we know that the definition is external, perhaps we could define the dictionary in more general terms, like say:

`@wdio/types`: `https://github.com/webdriverio/webdriverio/tree/main/packages/wdio-types/`

However, to link to a specific line on GitHub, the external library would need to provide declaration maps.
I think it's doable, but might be tricky.

If on top of declaration maps, the external package also defined the directory in their repository entry in package.json, the whole thing could work even without the explicit dictionary. 🤔

@B4nan
Copy link
Contributor Author

B4nan commented Aug 21, 2022

Good idea, I like the @external approach.

It would be also great to have a way to hide some of the external symbols (currently its either everything or nothing), but I guess that's more of a typedoc feature request. Like with the SessionPool class example which extends EventEmitter, there is too much bloat, we would be fine with having just a few methods there (e.g. emit), most of that is out of scope.

@B4nan
Copy link
Contributor Author

B4nan commented Feb 8, 2023

Can we at least hide the buttons if the symbol is external? Sounds better than rendering broken links and should be an easy fix, happy to send a PR.

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

No branches or pull requests

2 participants