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

fix(compiler-cli): pass config path to ts.parseJsonConfigFileContent #29872

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@filipesilva
Copy link
Member

commented Apr 12, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The config path is an optional argument to ts.parseJsonConfigFileContent. When passed, it is added to the returned object as options.configFilePath, and tsc itself passes it in.

The new TS 3.4 incremental build functionality relies on this property being present: https://github.com/Microsoft/TypeScript/blob/025d82633915b67003ea38ba40b9239a19721c13/src/compiler/emitter.ts#L56-L57

When using The compiler-cli readConfiguration the config path option isn't passed, preventing consumers (like @ngtools/webpack) from obtaining a complete config object.

Related to angular/angular-cli#13941.

What is the new behavior?

This PR fixes this omission and should allow JIT users of @ngtools/webpack to set the incremental option in their tsconfig and have it be used by the TS program.

I tested this in JIT and saw a small decrease in build times in a small project. In AOT the incremental option didn't seem to be used at all, due to how ngc uses the TS APIs.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@filipesilva filipesilva requested a review from angular/fw-compiler as a code owner Apr 12, 2019

@googlebot googlebot added the cla: yes label Apr 12, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Apr 12, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Apr 15, 2019

fix(compiler-cli): pass config path to ts.parseJsonConfigFileContent
The config path is an optional argument to `ts.parseJsonConfigFileContent`. When passed, it is added to the returned object as `options.configFilePath`, and `tsc` itself passes it in.

The new TS 3.4 [incremental](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html) build functionality relies on this property being present: https://github.com/Microsoft/TypeScript/blob/025d82633915b67003ea38ba40b9239a19721c13/src/compiler/emitter.ts#L56-L57

When using The compiler-cli `readConfiguration` the config path option isn't passed, preventing consumers (like @ngtools/webpack) from obtaining a complete config object.

This PR fixes this omission and should allow JIT users of @ngtools/webpack to set the `incremental` option in their tsconfig and have it be used by the TS program.

I tested this in JIT and saw a small decrease in build times in a small project. In AOT the incremental option didn't seem to be used at all, due to how `ngc` uses the TS APIs.

Related to angular/angular-cli#13941.

@filipesilva filipesilva force-pushed the filipesilva:ts-3.4-incremental branch from c65bd7b to d2f0592 Apr 15, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Apr 15, 2019

@filipesilva filipesilva requested a review from alxhub Apr 15, 2019

@alxhub

alxhub approved these changes Apr 16, 2019

@ngbot

This comment has been minimized.

Copy link

commented Apr 16, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@alxhub alxhub closed this in 86a3f90 Apr 16, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

fix(compiler-cli): pass config path to ts.parseJsonConfigFileContent (a…
…ngular#29872)

The config path is an optional argument to `ts.parseJsonConfigFileContent`. When passed, it is added to the returned object as `options.configFilePath`, and `tsc` itself passes it in.

The new TS 3.4 [incremental](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html) build functionality relies on this property being present: https://github.com/Microsoft/TypeScript/blob/025d82633915b67003ea38ba40b9239a19721c13/src/compiler/emitter.ts#L56-L57

When using The compiler-cli `readConfiguration` the config path option isn't passed, preventing consumers (like @ngtools/webpack) from obtaining a complete config object.

This PR fixes this omission and should allow JIT users of @ngtools/webpack to set the `incremental` option in their tsconfig and have it be used by the TS program.

I tested this in JIT and saw a small decrease in build times in a small project. In AOT the incremental option didn't seem to be used at all, due to how `ngc` uses the TS APIs.

Related to angular/angular-cli#13941.

PR Close angular#29872
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.