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

TypeDoc should error/warn when docs reference an unexported type #1653

Closed
pimterry opened this issue Aug 13, 2021 · 8 comments
Closed

TypeDoc should error/warn when docs reference an unexported type #1653

pimterry opened this issue Aug 13, 2021 · 8 comments
Labels
enhancement Improved functionality

Comments

@pimterry
Copy link

pimterry commented Aug 13, 2021

Problem

I've just updated to 0.21 from 0.17. I've switched to the new system, and lots of my types are referenced from my docs, but not included in my docs, because although they're exported from their own individual files they're not exported from the entrypoint itself.

For example:

entry-point.ts:

export { myFunc } from './my-func';

my-func.ts:

export type MyParam = number | string;

export function myFunc(param: MyParam) {
  // ...
}

Running typedoc ./entry-point.ts produces documentation for myFunc, which references MyParam but doesn't actually include it, which isn't very useful to anybody.

Personally, I'd prefer it if types referenced like this were exported, but I understand that that's not the current setup. I'm currently working to ensure that I do export all my types, but I'm worried that I'll miss some, or that in future I'll add new types internally that are eventually exposed via some exported API, but aren't themselves exported, and I'll forget to manually export them.

Suggested Solution

It would be great if mistakes like this were easier to catch.

TypeDoc could do this easily, since it clearly knows when it happens (it generates a reference to a type name that's not a link).

I would like an option to make the above fail the docs build.

I think this is primarily relevant for function parameters and return types. You could extend it to everything, e.g. base classes, but I don't think those matter as much: parameters & return types are the types that end users actually need to understand. YMMV though, that's debatable.

I think this should only apply to types within this codebase: types that come from node_modules somewhere don't have this problem (I don't want to re-export half of @types/node, every user of my module knows what a Buffer is).

Even better: I'd love an option that automatically included referenced types like this, but my impression is that they're excluded by design so that's not likely.

Search Terms

warning missing export forgotten implicit type dependency

@pimterry pimterry added the enhancement Improved functionality label Aug 13, 2021
@pimterry
Copy link
Author

Note that while this is similar to --listInvalidSymbolLinks, it's not the same: that doesn't list references like this that are missing, so for the example above it doesn't print anything.

@Gerrit0 Gerrit0 added good first issue Easier issue for first time contributors help wanted Contributions are especially encouraged labels Aug 13, 2021
@Gerrit0 Gerrit0 added this to To Do in Version 0.22 via automation Aug 16, 2021
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Aug 16, 2021

This is a such a good idea... I can't believe this is the first time I've seen it suggested! TypeDoc 0.22.0-beta.2 includes warnings for this. You can make the build fail by setting treatWarningsAsErrors.

@Gerrit0 Gerrit0 moved this from To Do to Done in Version 0.22 Aug 16, 2021
@Gerrit0 Gerrit0 removed help wanted Contributions are especially encouraged good first issue Easier issue for first time contributors labels Aug 16, 2021
@pimterry
Copy link
Author

Amazing response, thanks @Gerrit0! It looks like 0.22.0-beta.2 isn't published to npm yet, only 0.22.0-beta.1, but let me know as soon as it's available and I'd love to help testing 👍

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Aug 17, 2021

Shoot, sorry about that! Looks like the publish CI job failed and I didn't notice. Published now.

Doing this work, and then running it on typedoc itself has made me see that there's still some improvements needed here. Sometimes you really don't want to export something, so the warning is noise... and, now that I've done this, making typedoc document non-exported symbols isn't very difficult... even though I still don't like it. Still thinking about names...

notExportedSymbols: warn (default)
notExportedSymbols: document (this value doesn't seem discouraging enough, ideally I want something that implies that you're doing something bad... dangerouslyDocumentNotExported?)

Documenting not exported symbols today will break a ton of things - whenever there's more than one non-exported symbol that is referenced with the same name...

I also think a intentionallyNotExported list of names is important, for example S in TypeDoc's documentation is an alias that mostly just copies properties. It's not even exported from the file it is declared in... and I'm not convinced it should be. https://typedoc.org/api/interfaces/JSONOutput.ArrayType.html

@pimterry
Copy link
Author

Ok, I've got it working now! Yeah, so for my case, there's a couple of unnecessary warnings here I'd want to avoid. Maybe the examples will be useful:

  • A couple of base classes, which (imo) isn't important info for API consumers. Any issues where inherited methods are missing types are reported separately anyway, so this doesn't need a warning. I think your S type also falls into this bucket.
  • Some external type definitions, e.g.:
    • Warning: "Socket" defined at node_modules/@types/node/net.d.ts:74 is referenced but not included in the documentation.
    • Warning: "Function" defined at node_modules/typescript/lib/lib.es5.d.ts:271 is referenced but not included in the documentation.
  • Various internal-use-only methods, which actually shouldn't really be included, so I'm going to remove them from the exported docs entirely.

That said, while that noise is a problem, the other 50% of the warnings are real issues I was unaware of, and which were super easy to fix manually, so this is really useful to me already!

Personally, if the logic changed so that node_modules types where excluded and only relevant API types were included (field types, method parameters, method return types... that's it?) then I think the current warnings would work perfectly. Do those sound like reasonable rules in general?

One other thing: is it easy to show where a type is referenced in the warning? I'm currently having to grep through the output to find the source of the missing symbol.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Aug 19, 2021

Thanks for the feedback!

The first version had the node_modules check, but apparently using some instead of every for symbol locations - swapped now. Beta 4 adds a intentionallyNotExported option to suppress warnings for the given reflections. I want to keep the checking for base types too.

Including the location is also fairly easy, I added that in beta 4 as well.

@pimterry
Copy link
Author

All working great for me now! I've used intentionallyNotExported to drop my base classes and that works perfectly. Thanks again! I'll close this issue now 👍

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Sep 4, 2021

I want to keep the checking for base types too.

I changed my mind. Doing this means that it isn't possible to do things like what TypeDoc does with S, removed in 0.22.0-beta.8, I also added a warning for intentionallyNotExported items which are never used to prevent a warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improved functionality
Projects
No open projects
Development

No branches or pull requests

2 participants