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

Support ESM and functions/async functions in config files #2268

Conversation

cprussin
Copy link
Contributor

Support ESM in config files. Also support the default export of config files being any of the following:

  1. The config itself (current behavior)
  2. A function that returns the config
  3. A function that returns a promise that resolves to the config (or an async function that returns the config)

@cprussin cprussin force-pushed the support-esm-and-functions-and-promises-in-config branch 2 times, most recently from e4584b7 to 4f619fe Compare April 30, 2023 04:38
@cprussin cprussin force-pushed the support-esm-and-functions-and-promises-in-config branch from 4f619fe to 8e0d8e7 Compare April 30, 2023 04:45
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Apr 30, 2023

This is a major breaking API change for what feels like a very small benefit, if any to me...

  1. Application.convert is intentionally synchronous as it should be completely repeatable across runs, including ordering. It seems to be unrelated to the config file changes.

  2. Why does configuration need to be created in an async manner? Surely nobody is fetching stuff from a server for config... JS config files itself is a feature I've considered removing. I have yet to see a case where it is better to have a JS config than a simple JSON one, particularly since typedoc added support for comments and VSCode accepted a PR to automatically tie the json file to the config schema.

@cprussin
Copy link
Contributor Author

cprussin commented Apr 30, 2023

Apologies @Gerrit0 , I should have opened an issue first to discuss this, I didn't expect this to be controversial and didn't realize that the API was considered public or was intentionally synchronous.

So, in terms of why it's useful to be async, the immediate use case I'm having was to set up for a PR I'm planning to follow this one with, which was to add support for passing marked extensions. There's some that I'd like to use, and at least one (https://github.com/markedjs/marked-gfm-heading-id) -- which will fix some broken links I'm getting within my markdown files -- is distributed as ESM only and thus the config file would necessarily have to at least support async or ESM (but if we have to support one or the other there isn't any reason not to support both as either imply async config loading).

The other potential use case here which would make my life easier, but is not a necessity, is generating some options by introspecting on the codebase itself. Simple example here is that markedOptions.baseUrl needs to be different for each monorepo package, and I'd prefer not to have a manual config in each package. Another example is driving navigationLinks off values present elsewhere. Again, not as important, but I would still find some nice value in it.

Anyhow, no hard feelings if you'd prefer to just close this and then I won't move forward with adding the marked extensions PR. Let me know your thoughts.

For what it's worth, the only reason that convert had to become async was because it could return _convertPackages(), which in turn had to become async because it calls opts.read on the package dirs. Maybe there's another way to structure this such that loading the config can be async but _convertPackages() can remain sync?

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Apr 30, 2023

Simple example here is that markedOptions.baseUrl needs to be different for each monorepo package

Sorry to say that this won't fix your issue - marked is only invoked when actually rendering the site - that is to say, once, after all projects have been converted, so it will only have an effect in the root config.

Marked extensions could be achieved the same way TypeDoc's plugins are done - give TypeDoc a list of strings to require/import within MarkedPlugin in a function passed to Renderer.preRenderAsyncJobs

Application.convert I suppose could become async, so long as Convert.convert remains synchronous we won't have issues with nondeterministic builds... unfortunately, making Application.bootstrap async makes it impossible to run typedoc without introducing an async call, or re-implementing that logic (this is the big thing that breaks everyone that uses the API, and is the primary reason bootstrapWithPlugins was introduced in 0.24 to separate loading ESM modules out when adding support for ESM plugins)

On another note... markdown parsing could really use a huge overhaul. There's a pretty good argument for doing it when parsing comments themselves so that image links referenced in the comment can be tracked for copying to the rendered site (and maybe other links too?). I don't have anywhere near all the answers here, need to spend some time experimenting with what VSCode does to try to be consistent with it.

@cprussin
Copy link
Contributor Author

Thanks for the reply @Gerrit0 . I'll be honest, I know little enough about the typedoc ecosystem to have any valuable input here (I'm new to it myself so was just trying to help fix some pain points I ran into).

Most of my opinions around config file formats are informed by my experience with tools like eslint and jest, and I will say that in both those cases having the ability to use an async function to return the config turns out to be useful in a number of perhaps unexpected ways. Also, in both those cases, having strings to refer to the modules that then get required while loading the config turns out to be a major pain point -- mostly when dealing with shared configs and ensuring that the right dependency gets loaded. So much of a pain point that eslint is introducing a whole new config format for a few reasons, one big one being to resolve this issue.

So, I have a gut reaction to your suggestion of passing in a list of strings to require, and a gut instinct on what I think feels good for configuring stuff, but I also don't have enough experience with typedoc itself to really feel confident in my gut instinct. Maybe it's food for thought, maybe not.

In any case, I'm happy to keep contributing back! Happy to keep the PR open but put it on ice, happy to refine it down until it feels like the right solution if you're willing to advise me on what you think that right solution would be, or happy to just close it out, whatever is your suggestion.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 5, 2023

Gave this a few days to stew and I'm begrudgingly coming around -- I think this is a change that probably needs to happen. It definitely needs to wait until 0.25 though, which is realistically probably a couple months away. (The feature I want to release for 0.25 is proper inclusion of markdown docs in the generated docs, which incidentally will involve doing markdown parsing earlier and likely resolve your relative docs link issue... but I haven't really even started this besides thinking about it)

I very well might steal ESLint's config strategy and drop support for most if not all of the weirdness in configuration that typedoc does today. It's not as bad as ESLint was/kind of is, but it's still more complicated than I wish it was!

@cprussin
Copy link
Contributor Author

cprussin commented May 5, 2023

Makes sense @Gerrit0 . What can I do to help? Would you like to cut a branch somewhere that I can merge this over to or would you prefer to just sit on this until the time comes?

@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 5, 2023

I'll look at merging it into the beta branch next time I update it from master, tend to do that whenver working on it and there has been a release, so likely either this week or next week

@Zamiell
Copy link
Contributor

Zamiell commented Aug 1, 2023

Any updates on this? I'm eagerly awaiting being able to switch my TypeDoc configuration files to ESM, as it is one of the last CJS things now that ESLint has released its new config file format.

@cprussin
Copy link
Contributor Author

cprussin commented Aug 5, 2023

I'd still be more than happy to fix this up and get it merge-ready if @Gerrit0 wants to move forward, but last we chatted it didn't sound like he was quite ready to commit to wanting to move forward on this. I'm not sure if that's changed but I'm still available to help if so!

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Aug 13, 2023

This is probably the one breaking change I want to include in 0.25, along with bumping the minimum Node version since several dependencies have started requiring Node 16 for the latest version. I wanted to get more into this release, but haven't had the time...

Not sure I'll get this done in time to do the update as a part of the TS 5.2 release updates, but I'd like to..

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Aug 25, 2023

I've decided to not bring in the functions for configuration, just ESM config, since I think the right direction for TypeDoc's config is eventually the same as ESLint went - https://eslint.org/blog/2022/08/new-config-system-part-1/

@Gerrit0 Gerrit0 closed this in 5c977ae Aug 25, 2023
@cprussin
Copy link
Contributor Author

@Gerrit0 nice! Thanks for moving forward on this!

However, worth noting that while eslint's flat config does not support functions, it does support the config object being a promise, see the last paragraph here, which effectively enables all cases where a function / async function would be useful.

So I think if you're trying to follow eslint here it would be better to support promises too. And it's really useful to do so too since you never know when someone might need to do something dynamic while generating their config, for instance an async import of a shared config, and supporting promises makes such cases possible without weird hacks.

That said it's totally possible to accomplish the same thing with top-level await in ESM, although that only will work for folks using ESM configs and not cjs configs...

Anyhow, I don't have a use case for it so it doesn't affect me, but figured I'd share given my experience having needed this kind of thing in eslint in the past.

@cprussin cprussin deleted the support-esm-and-functions-and-promises-in-config branch August 25, 2023 19:23
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Aug 25, 2023

Easy enough to add two awaits there, I'll do that.

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.

3 participants