Default options for a custom option are overridden then deleted when some, but not all, options are set in typedoc.json

[citkane]

Search terms

options addDeclaration custom

Expected Behavior

When adding custom options with app.options.addDeclaration, the defaultValue fields will be overridden by values in typedoc.json, otherwise retained as per default.

Actual Behavior

If some values are defined in typedoc.json, these override the defaults and the remainder are deleted. If an empty object for the custom key is defined in typedoc.json, all values are deleted. If no custom key is defined in typedoc.json, all values are retained as per defualt values.

Steps to reproduce the bug

minimal repro pr minimal repro

Environment

• Typedoc version: 23.10
• TypeScript version: 4.7.4
• Node.js version: 16.16.0
• OS: Ubuntu 22.04

[Gerrit0]

This is working as expected for mixed options. Mixed options could be objects, numbers, strings, or even a function... for this reason, TypeDoc performs no processing on them. Your plugin will need to do something like:

const opts = { ...app.options.getValue("opt"), ...defaults }

If your object only contains boolean flags, a Flags option type might be a better fit, TypeDoc does do some additional processing for that case.

[citkane]

Thank you - your suggested workaround is appreciated.

In practice, default options will need to be read in the declaration scope before they are lost, and then merged after the reader is run;

app.options.addDeclaration({...});
const defaultOptions = app.options.getValue("opt");

// this below, or in a new scope after app.bootstrap eg. after a renderhook
app.options.addReader(new TypeDocReader());
app.options.read(new Logger());
// end

const myScopedOptions = {...defaultOptions, ...app.options.getValue("opt")}

From the theme / plugin developer's perspective, the API method of app.options.getValue is now redundant for custom keys. For me, this feels verbose and messy.

However, given that;

• when a custom option key IS NOT passed in typedoc.json, the default values ARE retained,
• when a custom option key IS passed in typedoc.json, the default values are ARE NOT retained

I assert that the present behavior for mixed options is inconsistent.

I expect that there are many reasons why you are implementing the behavior this way, but I argue:

• for a theme / plugin developer this level of the API should behave consistently with no workarounds required.
• my intuitive expected behavior at this level of the API is that default values are retained / overridden, and that lower level concerns / complexities are addressed outside of the higher level API.

[Gerrit0]

I would instead recommend:

app.options.addDeclaration({ ..., default: {} }) // no real point including defaults here
// later...
const options = { foo: "a", bar: "b", ...app.options.getValue("opt") }

but yes, you have run into a use case which simply doesn't exist for TypeDoc itself, so additional code was not written to cover it. Instead of adding a single test option if adding something like this to TypeDoc, I would define testFoo and testBar string options so that the options could both be set on the command line, which would result in them being set separately, and thus not needing this merge behavior.

[citkane]

Appreciated, I will not be offended if you want to change this issues label from "bug" to something else, maybe "enhancement"?

I also appreciated that, if so required or desired, each option can be individually declared as strings for param parsing. For my purpose, this is not required, and not desired as it pollutes the top level options, and keyed sub-options are friendlier to document and consume.

IMO, issues like this one become important for a Typedoc extension's code readability and maintainability. I feel options are best declared consistently within the entry hook point, ie. load(). Down the line, your suggested method of declaring the options could become a needle in a haystack...

Here is how I am implementing a workaround, which I think is more elegant and succinct for future humans.

I am busy kicking the proverbial Typedoc tyres with some plugin development, and then a custom theme development. Thanks for the product, I see that you are doing a lot of heavy lifting here Gerrit0.

[Gerrit0]

I'm not really opposed to adding a new option type for nested options (how does it merge? don't want to add a new dependency, is Object.assign good enough? nested? does it require a similar object structure?) or reworking options entirely to be based on a deep merge mentality (would reduce the option soup that TypeDoc's own options have become...) but don't see getting to it anytime soon. PR welcome if you'd like to tackle either of these, though we probably need to work out a proper design if going after the larger option.

[citkane]

Agreed.

I am starting into a custom theme, which will improve my familiarity with the TDoc codebase.

I will come back here for discussion as and when ideas emerge.

[citkane]

Current Fix

I have created a Pull Request which works around this issue.

It does nothing more than handling 'mixed' type options for plugins by stashing defaults at the beginning of bootstrap() and merging them back at the end.

Breaking future updates

More elegant solutions are possible, but they are going to introduce breaking changes. I have started working on this here. The basic approach:

• use the typeDoc 'events' architecture to break bootstrap() into stages with hooks.
• using the event hooks, explicitly declare the 'read' order
• allow hooks a handle on app to read/modify state along the chain.

This will provide a more readable and flexible architecture for plugin/theme developers, and also deprecate some of the obfuscating functions around the read order and double reading of args.

This is a bit messy for now, but does function although it wreaks havoc with the whole testing landscape...

[Gerrit0]

I think both of the proposed options are making this more complicated than it needs to be. A non-breaking way of introducing this behavior that I'd rather go for is:

• Leave Mixed options as is
• Introduce a new ParameterType.Object
• Update convert in utils/options/declaration.ts to accept a new oldValue argument, which can be used by the Object option type to merge the old and new objects into the converted value.

[citkane]

I do appreciate your concerns, and understand that you prefer adding a new behaviour instead of risking interference with existing behavior. This makes sense.

I understand your proposal to be as per below:

1. The only directly scoped influence a plugin can have on it's options is inside the main application bootstrap(), within the loadPlugins() call.
2. Outside of the control of the plugin, inside bootstrap() and after loadPlugins(), options.read() is called, which chains through the TypeDocReader to readFile to the code below.

        for (const [key, val] of Object.entries(data)) {
            try {
                container.setValue(
                    key as never,
                    val as never,
                    resolve(dirname(file))
                );
            } catch (error) {
                ok(error instanceof Error);
                logger.error(error.message);
            }
        }

3. The above calls setValue(), which contains the call to convert in utils/options/declaration.ts
4. Behaviour is now contained in the new ParameterType.Object. When setValue() is called on it by;

• the plugin author within the initial scope of the plugin, or
• by the main application in (2)

The default options will now be overridden, and not deleted, in both cases above.
This is a safe fix to the stated issue.

Concerning the extensibility of future versions of typeDoc (which may be breaking), the API available to a plugin remains restrictive and inconsistent.

There could be any number of reasons why a plugin author may want to, within the bootstrap scope (before the application runs), conditionally and selectively override not only their own, but also the other application options.

In this case, they would expect:

• Modifications to any and all options would persist into run().
• the Object and Mixed types to behave consistently (potentially raising confusion about why the 2 different types exist in the first place)
• have the ability to perform array functions (push, filter, etc) and persist modifications on array type options.

I think that much of this can be achieved in a non-breaking way by adding event hooks into the bootstrap process.

I suggest considering if your proposal for ParameterType.Object could not be applied to the existing ParameterType.Mixed without breaking anything, and negating the need to create a new parameterType?

[Gerrit0]

Modifications to any and all options would persist into run().

They would only expect this if they had not read the overview of how typedoc works. That is not how it works, and should not be how it works since it means inconsistent behavior for plugins stomping on options. (Which is a bad idea anyways...)

the Object and Mixed types to behave consistently

I disagree here... You shouldn't expect StringArray and PathArray options to behave the same way, despite both being able to receive an array. There's a meaningful distinction between these, same for Object and Mixed.

have the ability to perform array functions (push, filter, etc) and persist modifications on array type options.

They can do this today, but they have to place a hook in the appropriate place. I would be okay with an event emitted at the end of Application.bootstrap to make this easier, but really dislike using events for control flow (e.g. "reader init", "reader do read") as it makes it much more difficult to follow the logic without having to debug code.

[citkane]

This is not about reading the overview, or any other helpful resources.

Typedoc appears to (correctly) make the parsed options immutable in app.convert() close to the top of run(app), immediately after bootstrap().

Given that, within bootstrap, options.reset() is called after plugins are loaded, there is no existing appropriately placed hook before options are frozen.

• An extension developer (the user) has no control over this.
• I agree that the end of Application.bootstrap is the best place for a new hook.

graph LR;
subgraph Bootstrap
Start-->loadPlugins;
loadPlugins-->options.reset;
options.reset-->options.read;
options.read-->End;
end
subgraph run
subgraph app.convert
End-- no existing hooks -->options.freeze;
end
options.freeze-->A[complete run]
end
B{New Hook?}-->End;
loadPlugins-- the user has no control -->options.freeze

IMO, any documentation software package should make the user a first class citizen. The default theme and documenting style / architecture can be as opinionated as desired in the core, but the extensibility should be equally flexible.

Yes - allowing a plugin to manipulate options before they are frozen can break things, but equally so for any number of other things a plugin could do, and trying to control that will be like pulling the frayed end of a jumper thread.

Yes - there is a meaningful distinction between stringArray and pathArray in the purpose and behavior of how they are resolved, but this is not true for Object and Mixed.
Object is being proposed as a low risk workaround to substitute unanticipated and inconsistent behavior in Mixed when it is consumed by users.
Having both has the potential to confuse users, and the more robust approach is fixing the behavior in the existing type without compromising core.

IMO, this issue can be resolved by solutions already discussed:

1. A new hook at the end of bootstrap
2. Applying your aforementioned solution to the existing Mixed type, even if it may mean waiting for a breaking release.

Let me know if there is anything you want me to do about this directly.

[Gerrit0]

Plugins may add readers to the options object, so can technically modify values there... I agree the event at the end of bootstrap is a good idea.

but this is not true for Object and Mixed.

Here's where we disagree. Two examples in TypeDoc's options today:

• visibilityFilters
• searchGroupBoosts

Visibility filters have default values for protected, private, inherited, and external. Users can specify values for these to overwrite the defaults or they could specify { "@internal": true } to hide all of the default filters, and only include one for values tagged with @internal.

Search group boosts today defaults to an empty object, so it would be moot, but I've been considering adding a default to weigh top level reflections higher than properties/methods. You could argue that partial-overrides should apply here, but that either prevents TypeDoc from warning if you specify some custom group which doesn't exist (with @group) (it shouldn't show up for default groups since you shouldn't get a warning if you happen to document something without classes), or severely complicates the logic to provide that user experience improvement.

I'd be happy to merge a PR adding the hook to bootstrap + a new Object option type, but I don't want to put this functionality on Mixed options.

[citkane]

Ok, I can now see where changing the behaviour on Mixed will change existing core behaviour. Thanks.

IMO, the existing behavior you have clarified for "visibilityFilters" and intended for "searchGroupBoosts" is implicit, and I prefer explicit public API's, even if they may need to be more verbose. That's my 2 cent's worth and maybe a debate for another day! :smile:

I will get started on a PR for the new event hook and Object type.