Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(jsii): configure outDir and rootDir for tsc #593

Merged
merged 6 commits into from
Jul 12, 2019

Conversation

hoegertn
Copy link
Contributor

@hoegertn hoegertn commented Jul 9, 2019

This PR adds the possibility to configure the outDir and the rootDir for tsc

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@hoegertn hoegertn requested a review from a team as a code owner July 9, 2019 23:30
@hoegertn hoegertn changed the title feat(jsii): configure outDIr and rootDir for tsc feat(jsii): configure outDir and rootDir for tsc Jul 9, 2019
@@ -138,6 +140,8 @@ export async function loadProjectInfo(projectRoot: string, { fixPeerDependencies

excludeTypescript: (pkg.jsii && pkg.jsii.excludeTypescript) || [],
projectReferences: pkg.jsii && pkg.jsii.projectReferences,
tscOutDir: pkg.jsii && pkg.jsii.tscoutdir,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call it tscOutDir in the jsii config as well?

(thinking aloud) does this make sense in package.json or should they be command-line arguments? I guess we're mirroring the tsconfig.json here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I named it according to the outdir variable but I am happy to rename it. I think this is something I want to configure for my project. I don't think there is much difference in configuring it inside the jsii block or in the scripts section ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd like to have options that'll land in tsconfig.json modeled in a separate namespace altogether:

/// package.json
{
  // ...
  "jsii": {
    // ...
    "tsc": {
      "outDir": "dist",
      "rootDir": "lib",
    },
    // ...
  },
  // ...
}

Then, I'd use the exact same name that is in tsconfig's compilerOptions block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great. I will update this.

Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty good! I have a few minor comments, but I reckon this can be merged once they're addressed!


const prog = ts.createProgram({
rootNames: this.rootFiles.concat(_pathOfLibraries(this.compilerHost)),
options: COMPILER_OPTIONS,
options: {...COMPILER_OPTIONS, outDir: pi.tsc && pi.tsc.outDir, rootDir: pi.tsc && pi.tsc.rootDir},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be (since we decided to use the same name...):

{ ...pi.tsc, ...COMPILER_OPTIONS }

The order is important here - we don't want pi.tsc to be able to override the required settings we have in COMPILER_OPTIONS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now this would work, but if someone adds rootDir or outDir to COMPILER_OPTIONS manual overrides would no longer be possible. But I am fine with the change.

...COMPILER_OPTIONS,
noEmitOnError: false,
outDir: pi.tsc && pi.tsc.outDir,
rootDir: pi.tsc && pi.tsc.rootDir,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, here I'd just ...pi.tsc before ...COMPILER_OPTIONS.

@@ -188,11 +197,13 @@ export class Compiler implements Emitter {
lib: COMPILER_OPTIONS.lib && COMPILER_OPTIONS.lib.map(name => name.slice(4, name.length - 5)),
// Those int-enums, we need to output the names instead
module: COMPILER_OPTIONS.module && ts.ModuleKind[COMPILER_OPTIONS.module],
outDir: pi.tsc && pi.tsc.outDir,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here :)

@rix0rrr rix0rrr merged commit 21855e2 into aws:master Jul 12, 2019
@hoegertn hoegertn deleted the configure-tsc-out branch July 12, 2019 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants