feat(readers/tsconfig.ts): support "extends" #436

StrahilKazlachev · 2017-02-25T22:40:48Z

close #380

StrahilKazlachev · 2017-02-25T22:42:34Z

src/lib/utils/options/readers/tsconfig.ts

@@ -26,6 +26,30 @@ export class TSConfigReader extends OptionsComponent
    private static OPTIONS_KEY:string = 'tsconfig';


+    private readConfigFile(event:DiscoverEvent, fileName:string) {
+        if (!FS.existsSync(fileName)) {


@blakeembrey Not sure if this check is needed here, what do you think?

Probably not if it's already handled. However, is it possible to skip most of this code and have ts read the entire file with extends instead? I thought there was a method for it.

As I get it we need the raw compiler options/content. parseJsonConfigFileContent() does follow the extends chain and aggregate the data but not in the raw form. If we use the options: CompilerOptions property from the result of parseJsonConfigFileContent() we will have to remove all internal options and map the enum represented ones(ModuleKind, ScriptTarget, NewLineKind, ...) to the correct string values. Don't like the maintenance perspective or using APIs marked as internal.

Do you know where we use the raw compiler options? My primary concern is that we may implement extends "wrong" if the compiler options ever change. If I've learnt anything from using the TypeScript compiler, it's that things can change at any time so implementing your own is usually bad.

I'm not very familiar with the project so here is how I've understood it so far:

Here a typedoc intended only options may be present in the tsconfig.json, those options get merged with the raw compilerOptions from the tsconfig.json.

The conversion of the raw options to CompilerOptions and separation - TS/TypeDoc , is done here by OptionDeclaration.

Why the conversion is done by typedoc and not TS I do not understand, I'm probably missing something or it is legacy. That's why I went with aggregating the raw options and not using the parsed ones returned from the parseJsonConfigFileContent() method used here - it returns ParsedCommandLine which has the parsed options - options:CompilerOptions.
If you have any suggestions please do tell.

I feel like it would be legacy and we should be able to tidy it up instead of trying to build our own extends. However, if there's too much to correct, let me know.

# Conflicts: # src/lib/utils/options/options.ts # src/lib/utils/options/readers/tsconfig.ts

blakeembrey · 2017-03-08T00:29:57Z

src/lib/utils/options/readers/tsconfig.ts

            data,
            ts.sys,
            Path.resolve(Path.dirname(fileName)),
            {},
            Path.resolve(fileName));

-        event.inputFiles = data.fileNames;
+        event.inputFiles = fileNames;
+        options = _.clone(options);


Should we care about cloning the options here? The config parse function should be generating a plain object that wouldn't be used anywhere else anyway.

Can you also point out where extends is now handled? I'm not sure I'm reading this snippet correctly, but it seems to move things around but I can't tell what/where something new was introduced.

extends is handled by ts.parseJsonConfigFileContent(). The rest of the changes here are for preserving the precedence of the options(cli, ..., tsconfig.compilerOptions) and clearing the unsupported options.

blakeembrey · 2017-03-08T00:33:05Z

src/lib/utils/options/options.ts

@@ -72,6 +74,10 @@ export class Options extends ChildableComponent<Application, OptionsComponent> {
        this.trigger(event);
        this.setValues(event.data, '', event.addError.bind(event));

+        if (event.compilerOptions) {
+            _.assign(this.compilerOptions, event.compilerOptions);


Should we use event.data for everything instead of having two ways to pass this data?

It can be used, but currently event.data is more like event.rawOptions so whatever additional data is added it should not be in conflict with any of the options and should be deleted before parsing them.
If the following is OK I'll make the changes:

in readers/tsconfig.ts - add parsedCompilerOptions: CompilerOptions to the event.data

in options/options.ts - delete parsedCompilerOptions from event.data before parsing

StrahilKazlachev · 2017-03-16T14:04:20Z

@blakeembrey ping

feat(readers/tsconfig.ts): support "extends" close TypeStrong#380

fd0c2d0

StrahilKazlachev commented Feb 25, 2017

View reviewed changes

StrahilKazlachev mentioned this pull request Feb 26, 2017

Add new tasks and expose them via a CLI aurelia/tools#51

Open

3 tasks

StrahilKazlachev added 2 commits March 3, 2017 15:34

let TS trace "extends"

907b732

Merge branch 'master' into feat/tsconfig/extends

f139c40

# Conflicts: # src/lib/utils/options/options.ts # src/lib/utils/options/readers/tsconfig.ts

blakeembrey reviewed Mar 8, 2017

View reviewed changes

StrahilKazlachev closed this Mar 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(readers/tsconfig.ts): support "extends" #436

feat(readers/tsconfig.ts): support "extends" #436

StrahilKazlachev commented Feb 25, 2017

StrahilKazlachev Feb 25, 2017

blakeembrey Feb 26, 2017

StrahilKazlachev Feb 26, 2017 •

edited

Loading

blakeembrey Feb 27, 2017

StrahilKazlachev Feb 27, 2017

blakeembrey Mar 2, 2017

blakeembrey Mar 8, 2017

blakeembrey Mar 8, 2017

StrahilKazlachev Mar 8, 2017 •

edited

Loading

blakeembrey Mar 8, 2017

StrahilKazlachev Mar 8, 2017

StrahilKazlachev commented Mar 16, 2017

feat(readers/tsconfig.ts): support "extends" #436

feat(readers/tsconfig.ts): support "extends" #436

Conversation

StrahilKazlachev commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrahilKazlachev Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrahilKazlachev Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrahilKazlachev commented Mar 16, 2017

StrahilKazlachev Feb 26, 2017 •

edited

Loading

StrahilKazlachev Mar 8, 2017 •

edited

Loading