fix(options): Increase argument options reader discover event priority

[pat841]

Previously the arguments reader would run after both the tsconfig and typedoc readers. However, both of those readers rely on event data such as "-options" that is parsed by the arguments reader which leads to issues when trying to point to a given config using a CLI argument.

[rklfss]

This is also necessary to give the input files of the tsconfig reader priority over the command line argument (that usually just points to the directory of the tsconfig file).

[blakeembrey]

/cc @thecjharries

[pat841]

Ill explain the issue a bit further:

In application.ts:bootstrap(), two option events get emitted:

this.options.read(options, OptionsReadMode.Prefetch);
this.options.read(options, OptionsReadMode.Fetch);

Each reader listens for the options discover event via:
arguments:

this.listenTo(this.owner, DiscoverEvent.DISCOVER, this.onDiscover, -200);

tsconfig:

this.listenTo(this.owner, DiscoverEvent.DISCOVER, this.onDiscover, -100);

typedoc:

this.listenTo(this.owner, DiscoverEvent.DISCOVER, this.onDiscover, -150);

That leads to the readers executing their respective onDiscover methods in the order: tsconfig, typedoc, arguments using the old priorities.

However, both the tsconfig and typedoc onDiscover methods check for reader.OPTIONS_KEY in event.data:

tsconfig:

    onDiscover(event: DiscoverEvent) {
        if (TSConfigReader.OPTIONS_KEY in event.data) {
            this.load(event, Path.resolve(event.data[TSConfigReader.OPTIONS_KEY]));
        } else if (TSConfigReader.PROJECT_KEY in event.data) {
            // The `project` option may be a directory or file, so use TS to find it
            let file: string = ts.findConfigFile(event.data[TSConfigReader.PROJECT_KEY], ts.sys.fileExists);
            // If file is undefined, we found no file to load.
            if (file) {
                this.load(event, file);
            }
        } else if (this.application.isCLI) {
            let file: string = ts.findConfigFile('.', ts.sys.fileExists);
            // If file is undefined, we found no file to load.
            if (file) {
                this.load(event, file);
            }
        }
    }

typedoc:

    onDiscover(event: DiscoverEvent) {
        if (TypedocReader.OPTIONS_KEY in event.data) {
            this.load(event, Path.resolve(event.data[TypedocReader.OPTIONS_KEY]));
        } else if (this.application.isCLI) {
            const file = Path.resolve('typedoc.js');
            if (FS.existsSync(file)) {
                this.load(event, file);
            }
        }
    }

This leads to the reader.OPTIONS_KEY arguments being parsed last:
arguments:

    onDiscover(event: DiscoverEvent) {
        if (this.application.isCLI) {
            this.parseArguments(event);
        }
    }

Another note. Would it be worth considering parsing the arguments during OptionsReadMode.Prefetch and the rest of the readers during OptionsReadMode.Fetch since the mode is essentially ignored?

@thecjharries A comment about #587, one solution is to enforce an --input option that specifies the space delimited files/paths to read from, rather than just files.push(arg); on any unknown argument.

[shlomiassaf]

I can confirm, #587 should be reverted or refactored.

Priority issues:

The commit introduces a lot of issues, both order execution issues and logical issues.
With the change the order is:

1. TSConfigReader (-100)
2. TypedocReader (-150)
3. ArgumentsReader (-200)

It does not make sense to fire TSConfigReader first as it relays on input for tsconfig file location from TypedocReader and/or ArgumentsReader, with this state the tsconfig file is always the one in the current directory.

TSConfigReader must be last

Logical issues

For some reason, ArgumentsReader will now override all files regardless.
Before: event.addInputFile(arg);
After: event.inputFiles = files

files is an array, and the process collect files from the arguments.
If no files it will still override the event.inputFiles

ArgumentsReader must add, not override.

I'm not aware of the reason for that change, but if it's to give the CLI args precedence for file selection it is a problem, if a user specify a tsconfig it should always take precedence as it is more specific.

Same thing for typedoc.js, if that is set it must always take precedence since it's more specific.

[shlomiassaf]

@pat841 you need to add more thing to the commit.
Revert ArgumentsReader back so it will append files, not override the whole array.

@blakeembrey As a guideline, I suggest locking ArgumentsReader to run first at all times, it does not make sense otherwise.

CLI arguments are used as the interface for controlling low level configuration, such as which tsconfig file to use or which typedoc.js file to use.
Other users might add custom readers, they might also require input which is possible only via cli arguments.

[shlomiassaf]

BTW, this should be done ASAP, it's hard to workaround.

[thecjharries]

@shlomiassaf: Re priority: the options are parsed twice, first using defaults, then second with values discovered from the initial read. That wasn't something I added. That's why it starts with TS, adds TypeDoc, and finishes with CLI. Re logic: in general, command line options override configuration (Google that phrase for some examples) because they're used to modify the base. Again, that's not something I made up. Similarly, any options from TypeDoc would be specified to replace base config.

[shlomiassaf]

If we want to follow that way it's fine but it needs to make sense.
Check the logic as is now, it is not possible to select a tsconfig path because the TsReader will always fire before the arguments.

in general, command line options override configuration

I'm fine with that, but with logic that fits our use case, we can't just follow someone's best practice without adaption to the specific use case.

If readers depends on CLI input, CLI must come first... that's simple logic.
If we want CLI args to override then set 2 event listeners - one at -1000 one at 1000 with different logic, one will handle initial setup of arguments for all other plugins and the other will close up with overrides where needed.

BTW, it is still required to add files and not remove, in ArgumentsReader... or at least skip if length is 0.

[thecjharries]

I'm not sure I understand your concern about the use case. Are you saying TypeDoc shouldn't use tsconfig.json or typedoc.js and should instead run off CLI flags only? That seems to be a step in the wrong direction. TypeDoc extends the base TypeScript configuration, and the CLI extends the base TypeDoc configuration.

As to your concern about the load order, I'm not sure you're following how TypeDoc parses options. Check out the original PR for a detailed explanation. Again, like a majority of tools running a stack of other tools, TypeDoc layers its config. It first finds a base tsconfig or uses system defaults. It then updates those options and adds TypeDoc-specific options with typedoc.js. It then updates all the options with CLI arguments. In theory, that means you could run three separate values for option files by putting different values in tsconfig, typedoc.js, and --files. In practice, TypeDoc is still a beta tool and many options are totally ignored or have been unintentionally altered, as is the case with files.

Edit: Think of it this way: tools that extend configuration have to start with something. You can either specify a subset of the options first and fill the missing options with defaults later, or start with everything and replace the defaults as you discover values. TypeDoc uses the latter approach. It's an easier solution programmatically, because you're simply setting members instead of performing diffs or checking if a value exists.

[pat841]

@thecjharries
The issue with parsing the CLI arguments last via ArgumentsReader is that both TSConfigReader and TypedocReader rely on CLI arguments which are unknown to them for their respective onDiscover events.

On top of that, BOTH options events OptionsReadMode.Prefetch and OptionsReadMode.Fetch are entirely separate events meaning even though ArgumentsReader which runs last during OptionsReadMode.Prefetch, event.data during OptionsReadMode.Fetch will reset to undefined.

@shlomiassaf I'm not entirely sure what you want me to change. This PR is only for modifying the reader orders.

[thecjharries]

@pat841 So you're saying the defaults loaded during Prefetch are being dropped? That makes sense; bootstrap isn't updating the options object. Couldn't that be resolved by changing the second call to use this.options instead of options? That's obviously a matter for another pull request, but it seems like that's the real issue here. (Edit: looks like this.options.getRawValues() might be the correct thing to use)

[pat841]

@thecjharries I'm all for that. I just assumed we dropped the event data for some specific reason. Want me to just update this PR and reset the priority change?

[thecjharries]

Edit: as the website politely tells me right below the box, email replies don't support markdown. Whoops. The properly compiled comment is immediately below.

[thecjharries]

Round two:
I think that's the right approach, because the PR seems to answer an XY problem. I'm mobile right now and can't pull a copy to play to with right now. However, I have a suspicion that breaking this would just involve setting values in both the CLI and tsconfig.

At the same time, I'm really worried that the change is going to generate a slew of new issues. By reusing the Prefetch data, it seems pretty likely that some unexpected behavior is going to pop up. For example, using your great analysis of the discovery modes as a starting point, Prefetch could assign keys that should have been updated but have been assumed to be empty based on the root bug behavior. A Prefetch event spawns a TypedocReader with option TypedocReader.OPTIONS_KEY(quick aside: that key is way too generic to be useful in a large setting). What I'm not sure about is if it's set after load extends data. A possible solution would be to removeDeclaration any assumed super values like that when in Prefetch.

It's 100% not working as intended, so the PR should happen. It's been that way for two years, so you bet that quirk is baked into a few things.

[thecjharries]

I've already found a potential issue. TsConfigReader destructures tsc.parseJsonConfigFileContent, keeping both options and raw. Assuming neither OPTIONS_KEY nor PROJECT_KEY are set on first run, onDiscover loads the closest tsconfig via ts.findConfigFile, which searches up the entire directory hierarchy. Because '.' is used as the current directory instead of some safe like process.cwd(), there's a really good chance it could pick up totally unrelated config. Even with a safe directory, there's a slightly smaller chance that you're leveraging multiple config files based on location and the nearest one isn't the right one. The error is going be very subtle, because you won't notice it for things you explicitly set in each tsconfig. Suppose Prefetch reads a tsconfig that includes "someOption": "not default value", and Fetch reads tsconfig that does not include the value at all. The returned options, extending Prefetch with Fetch, will have someOption !== "default value" && someOption === "not default value". Strange include options is what started me to investigate this in the first place with #587.

Actually using the Prefetch options is the right solution, but it's going to be ton of work to implement correctly. Stuff like that bug are going to take some serious work to figure out, and I have a feeling it will be some time before using the options correctly is stable.

[thecjharries]

Actually, this might much easier than I was making it. The readers are all very good at checking their specific options and falling back to defaults when nothing is found. Perhaps the defaults could just be skipped in Prefetch, starting Fetch with only the explicitly declared options. For example, TSConfigReader could easily be refactored to do nothing without (OPTIONS|PROJECT)_KEY on Prefetch (source):

// Import OptionsReadMode
import { OptionsComponent, DiscoverEvent, OptionsReadMode } from '../options';
...
// Update the last conditional in onDiscover
} else if (this.application.isCLI && event.mode === OptionsReadMode.Fetch) {

[rklfss]

In utils/options/readers/arguments.ts
if you replace

    private parseArguments(event: DiscoverEvent, args?: string[]) {
        ...
        
        if (files) {
            event.inputFiles = files;
        }
    }

with

    private parseArguments(event: DiscoverEvent, args?: string[]) {
        ...
        
        if (files.length > 0) {
            event.inputFiles = files;
        }
    }

you would be fine, at least regarding the input files issue.

Command line call could be

typedoc --tsconfig ./tsconfig.json

without any non-option arguments (that in fact would be treated as files by the arguments reader).

At the moment, if (files) is always truthy, at least an empty array. So if you use the mentioned command line call you will get an no-input-files error.

[pat841]

@thecjharries @shlomiassaf @makana Please check out the amended commit. I think this should solve most of our problems.

ArgumentsReader runs both during Fetch and Prefetch but after TSConfigReader and TypedocReader during Fetch.

[pat841]

Any update on this? This is still a breaking bug that needs to be fixed.

[aciccarello]

Looks good to me but I haven't tried it. My understanding is that typedoc does an initial "prefetch" to read the options (from the CLI?). Then it uses the values from those options to get the tsconfig and typedoc config files.

@pat841 @shlomiassaf @thecjharries @makana Have you all tested this on actual configuration? I'd like to get this out soon.

[pat841]

@aciccarello Yes, I have been using my own branch with this PR on our production systems, supports both CLI and config files.

[lddubeau]

@aciccarello I've used this patch in production. I cannot do any useful work with typedoc 0.9.0 unless I use a custom build that includes this patch.

Presumably I cannot use 0.10.0 either, which I see has been recently released with TS 2.7 support, but without this patch.