Fix tsconfig overridden by defaults #165
Conversation
Export optionDeclarations so that tests can reference them Parse cmd args without defaults to prevent overriding tsconfig values Apply defaults to the options last Support for spaces in project filepath: Remove double quotes from project file path when present
Missing default for a boolean arg is read as default: false This seems to be fixed in 12.0.1.
Command line args, tsconfig.json, and TSTL defaults is parsed properly.
|
If yargs really has these issues, we should probably stop using it. |
|
@lolleko Just one (no API for checking the origin of the value; args or default). But yeah, that's an option as well. |
| // options is a deep object, Object.assign or {...options} still keeps the referece | ||
| const copy = JSON.parse(JSON.stringify(options)); | ||
|
|
||
| const optionDefaults: yargs.Arguments = {_: null, $0: null}; |
There was a problem hiding this comment.
The typing of the object requires those as its yargs.Arguments (non-optional props). I didn't want to use as any later when they are returned as yargs.Arguments, although in this context it's probably fine.
yargs.Arguments type: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/yargs/index.d.ts#L227
I suppose one option is to use Partial<> and then force it to yargs.Arguments to retain at least some type info.
There was a problem hiding this comment.
Looks a bit strange but okay, I'm fine with it for now.
| const section = copy[optionName]; | ||
|
|
||
| optionDefaults[optionName] = section.default; | ||
| delete section.default; |
There was a problem hiding this comment.
I don't really like deleting, shouldn't the result be an empty object anyway?
There was a problem hiding this comment.
I'm not sure if I understand your question. What result should be an empty object anyway?
There was a problem hiding this comment.
Nevermind I missread, looks a bit strange but okay.
Command line arguments are parsed first with yargs. This causes the parsed args to contain the default values as well. Since tsconfig is parsed next, its values will be overridden by command line args, as they should. However since the command line args also include argument defaults, tsconfig can never properly set these.
Example:
tsconfig.json
Args
Previously resulted in luaTarget being forced to JIT.
I fixed this by parsing command line args without defaults & applying them last, since all args are applied that way (if it is already set, don't set it again).
I found a bug in yargs version 11.1.0: When a boolean argument doesn't have its default set, the default for that argument is still false (probably because undefined is falsy). Version 1.12.1 doesn't have this issue & seems to work.
There was also an issue with passing project with quotes (tsc supports this),
-p "tsconfig.json", which I fixed by checking for quotes.I think this is a difficult issue to solve "right" because you can't fix it by just changing the order of options merging. Since command line arguments are highest priority, they shouldn't contain default values. Yargs also doesn't include meta data about the parsed values (actually read from args vs set from defaults).
There's an issue but it doesn't seem to have resulted in a fix: yargs/yargs#513
Which is the reason I think parsing the args without defaults is probably the best solution. Another option would be to check the source of the value when adding command line arguments to the options, but that duplicates code (iterating & parsing args, which yargs already handles).