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

0.25.6 version fails to generate docs #2469

Closed
HarelM opened this issue Jan 1, 2024 · 8 comments
Closed

0.25.6 version fails to generate docs #2469

HarelM opened this issue Jan 1, 2024 · 8 comments
Labels
bug Functionality does not match expectation

Comments

@HarelM
Copy link
Contributor

HarelM commented Jan 1, 2024

Search terms

Expected Behavior

Build should pass

Actual Behavior

Build gets stuck

Steps to reproduce the bug

Build can be seen here:
This is part of a PR to automatically update dependencies to latest version:
https://github.com/maplibre/maplibre-gl-js/actions/runs/7376064163/job/20068383029?pr=3523

Let me know if this is enough.
Basically clone the repo, npm install, npm run generate-docs

Environment

  • Typedoc version: 0.25.6
  • TypeScript version: 5.x
  • Node.js version: 20
  • OS: linux (CI)
@HarelM HarelM added the bug Functionality does not match expectation label Jan 1, 2024
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 1, 2024

Please try 0.25.6, there was a bug introduced with .5 that caused failures similar to this

@HarelM
Copy link
Contributor Author

HarelM commented Jan 1, 2024

Thanks for the super quick response! I'll wait for dependabot update to pick this up and see if it gets fixed.

@HarelM
Copy link
Contributor Author

HarelM commented Jan 2, 2024

Seems like I was able to do it with dependabot recreate.
It looks like the latest version is stuck in an infinite loop.
I stopped the workflow run after 4 minutes when it usually takes 20 seconds.
Let me know if you need any help in reproducing the issue on the project I'm working on.
The project has some complicated types, so it might be related, nevertheless, it's a great way to test this lib :-)

Relevant PR:
maplibre/maplibre-gl-js#3530

@HarelM HarelM changed the title Latest version fails to generate docs 0.25.6 version fails to generate docs Jan 2, 2024
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 3, 2024

Been a while since I've seen an OOM on typedoc... Looks like this is the terribleness that is typedoc-plugin-not-exported.... Every time I have to fix a bug caused by that plugin doing terrible things I want to add more warnings to the top of the readme that it's a terrible idea to use it and you should export your public API...

@HarelM
Copy link
Contributor Author

HarelM commented Jan 3, 2024

I'm general, I believe our API is exported, having said that, there might be places where exported types or methods depend on non exported types, in this case if the option is between finding all these types and exporting them (although there are not really part of the public API) or using this plugin, it's an easy choice - use the plugin and save a lot of time and grief (for us).
While I really appreciate all the work you are doing here, and I don't want to sound ungrateful, I think this option should be part of this library, as a configuration, and not as a plugin.
But I might be wrong.
If this is something that is on the table I'll be happy to share examples where I couldn't find an elegant solution without using the plugin.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 8, 2024

You certainly aren't alone in thinking that it should be built in, I've been consistently resistant to this for a few reasons, not least among them that the slightly higher barrier to entry of installing a plugin results in people considering if some piece of their API really should be exported, which generally results in types being exported which make said library easier to use. There are legitimate uses for this capability, which is why the plugin exists, but I truly think that most libraries shouldn't need it.

Your specific case is uniquely problematic for TypeDoc, as it considers classes to be a "leaf" of the type hierarchy, not something that contains additional types/classes.... which doesn't play well with static Map = Map. Normally, I'd recommend getting rid of the class there, and using a plain module, but the custom accessors you have don't play well with that approach because of ESM's binding rules... I'd probably write them as plain get/setMaxParallelImageRequests functions, but that's a breaking API change.

Regarding the crash: With further review it looks like I made a bad assumption that it was the missing exports plugin causing this, the same bug can be triggered with a tiny amount of code:

export type ExpressionSpecification =
    | ["array", unknown | ExpressionSpecification]
    | [
          "array",
          string | ExpressionSpecification,
          unknown | ExpressionSpecification,
      ];

export class Map {
    getFilter(layerId: string): ExpressionSpecification | void {
        return;
    }
}

(This doesn't crash TypeDoc, but it does trigger TypeDoc's recursive type warning in 0.25.6, which really is something that shouldn't ever show up, and indicates something has probably gone wrong, or some edge case was missed. In this case, I accidentally inverted a check when refactoring in 0.25.5, and didn't notice so only half-fixed it in 0.25.6)

@Gerrit0 Gerrit0 closed this as completed in e84138c Jan 8, 2024
Gerrit0 added a commit that referenced this issue Jan 8, 2024
Gerrit0 added a commit that referenced this issue Jan 8, 2024
@HarelM
Copy link
Contributor Author

HarelM commented Jan 8, 2024

Thanks for the quick fix!
I'm not sure what should be the right solution honestly, I think I have a similar issue with dts-bundle-generator where types are not public due to how the API of this lib is defined (which has a long history, like everything bad in SW development 😔).
So I export them "manually" which might create problems in certain cases.
For example, you shouldn't create Style class, but you can get an instance if you call map.getStyle().
So type-wise you can define a variable of type Style, so the type should be exported, but not the class. Maybe an interface is the right approach here, IDK...
So I can't think of a proper solution for this...

@HarelM
Copy link
Contributor Author

HarelM commented Jan 9, 2024

Latest version seems to pass doc build and are producing new warnings (which is great! thanks!).
I'm finishing up the dependabot PR so latest version should be used in a matter of minutes:

Thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Functionality does not match expectation
Projects
None yet
Development

No branches or pull requests

2 participants