ts-unused-exports icon indicating copy to clipboard operation
ts-unused-exports copied to clipboard

Add optional config file

Open mrseanryan opened this issue 5 years ago • 5 comments

The number of options is growing, to the point where adding an optional config file sounds sensible.

Notes:

  • config file is optional
  • cmd line options should override the config file
  • avoid adding a library
  • support hierarchy of config files (like .gitignore) - useful for handling different groups of files (e.g. production vs test) - see #107
  • possibly support importing a base config file (to 1 level only)

mrseanryan avatar Jan 04 '20 09:01 mrseanryan

Cool.

I'll avoid a library if possible.

I'd do something like:

interface Options {
  //  our current options specifying optional keys like this:
  thisOneIsOptional: string | undefined;

  // rather than this:
  weShouldSpecifyLikeThis?: string;
}

const DEFAULT_OPTIONS: Options = {
  // default values for all our keys
};

const mergeOptions = (a: Options, b: Partial<Options>): Options =>
  Object.keys(a).reduce(
    (acc, k) => {
      const aValue = a[k];
      const bValue = b[k];
      acc[k] = bValue !== undefined ? bValue : aValue;
      return acc;
    },
    {} as Options,
  );

const commandLineOptions: Partial<Options> = whateverWeCurrentlyDo;

const configFileOptions: Partial<Options> = JSON.parse(readFileSync(configPath, 'utf8'));

const opts = mergeOptions(
  mergeOptions(DEFAULT_OPTIONS, configFileOptions),
  commandLineOptions,
);

(note that I just coded this snippet in this comment, never tested it, probably doesn't work without some tinkering).

pzavolinsky avatar Jan 04 '20 15:01 pzavolinsky

Looks good!

Question - what is wrong with using the ? thing - I thought that would be equivalent?

  // rather than this:
  weShouldSpecifyLikeThis?: string;

Thanks Sean

mrseanryan avatar Jan 04 '20 22:01 mrseanryan

what is wrong with using the ? thing - I thought that would be equivalent?

There is nothing wrong, and yes, both are almost equivalent, except the following:

interface WithQuestionMark {
  a?: string;
}

interface WithoutQuestionMark {
  a: string | undefined;
}

const x1: WithQuestionMark = {}; // OK
const x2: WithoutQuestionMark = {}; // ERROR
const x3: WithoutQuestionMark = { a: undefined }; // OK

In other words, by defining Options without ?, our DEFAULT_OPTIONS will always have all the possible option keys. This ensures that the mergeOptions function always iterates over every possible option.

If we had the ? version for say a?: string and in our DEFAULT_OPTIONS we omit that a value, but later specify a in the config file, that value for a will never make it into the final opts (because we only iterate keys that are present in the left most object and, in this example, a would not be present in DEFAULT_OPTIONS).

Using the | undefined form ensures that when we add a new option in the future, even an optional one, the compiler will complain that we missed setting a default in DEFAULT_OPTIONS and that will avoid us bugs.

Might be worth it to put a comment in the Options type to be explicit about not using the ? syntax there.

pzavolinsky avatar Jan 06 '20 00:01 pzavolinsky

I see - thanks for the explanation!

mrseanryan avatar Jan 06 '20 07:01 mrseanryan

Have updated the text at the top.

mrseanryan avatar Jan 19 '20 10:01 mrseanryan